Add updateMapStyle prop

[macobo]

This prop controls if map style should be updated on changes. Also
whilst this is true panning is disabled.

You might set this to false whilst integrating with mapbox-gl-draw.

The work is based off of the following issues:

• Discussion: integration with mapbox-gl-draw #725 - @ibgreen pointed out that this sort of prop would make for an
  easy integration.
• DrawControl #450
• Event handling of plugins #328

Context

At eAgronom, we tried out react-map-gl-draw but were forced to abandon it as it didn't yet support our needs. Re-reading the issues linked above, we've managed to build out a custom mapbox-gl-draw integration by:

• forking this library and adding the updateMapStyle prop.
• creating a custom react wrapper component which sets the following props on base map: { updateMapStyle: false, dragPan: false, onNativeClick: undefined, }
• adding the following css to .overlays whilst drawing: pointer-events: none

Questions to maintainers

I couldn't figure out how best cover this change with unit tests. We use enzyme for similar purposes. Any pointers?

[Pessimistress]

What if you just make mapStyle change shallowly? A "trigger" prop like this is anti-pattern in React, e.g. you have to manually switch it off and on again if you want it work a second time.

[macobo]

Thanks for the quick response @Pessimistress.

Not sure what you mean by "make mapStyle change shallowly?".

---

Some conjecture below, please correct me if I'm totally off-topic:

Perhaps you meant handling it outside of the component, in-app or within a stateful wrapper component for InteractiveMap that listens when props change and if some flag is true, forwards them. This could work, however:

1. The component is non-trivial in implementation as it would need to know about the internals of the thing it's wrapping (thus making it hard to maintain long-term).
2. As this change handles more than mapstyle changes (e.g. viewport change is also covered)
3. It would make using a mapbox-gl-draw style react wrapper hard to make usable. Users either couldn't use their existing <InteractiveMap /> and instead rely on library-provided alternatives OR would need 3 layers of components nested (e.g. Draw -> StatefulWrapper -> InteractiveMap).

All this seems like a lot of complexity for a relatively simple feature.

Also note that we're interested in open-sourcing our Draw wrapper (it's now in production) once its implementation has stabilized a bit. This hopefully gives a bit more weight to argument 3.

Aside: Point 2 makes me think that maybe the prop is currently misnamed - happy to iterate on this as needed if there's interest in getting this merged.

[Pessimistress]

Maybe I'm misunderstanding your use case. Can you share a code sample of the use case that you want to address?

[macobo]

So we're looking to create a wrapper component to use mapbox-gl-draw together with this library.

This is one of the most-requested features from this library and me and my engineering team has spent a lot of time investing different solutions that would allow us to make use of both react-map-gl as well as giving us the capability for drawing on the map.

A lot of the context is in #725, the main problem with this is that that mapbox-gl-draw does updates to the mapbox-gl object directly, in ways that are not reflected for react-map-gl. In essence, the two libraries are in conflict, causing a trashing effect where one undoes the work of the other.

Ibgreen suggested the solution that's in this PR 7 months ago - if we can turn off the updates, there's no thrashing. We tested it out and it works beautifully - we can draw on the map with the changes in this PR. :)

For reference, here's our current POC MapDraw component:

import React, { PureComponent } from 'react'
import { isEqual, map, noop, sortedUniq } from 'lodash'
import { Control } from 'mapbox-gl'
import classNames from 'classnames'
import MapboxDraw from '@mapbox/mapbox-gl-draw'

type DrawingMode = 'simple_select' | 'direct_select' | 'draw_line_string' | 'draw_polygon' | 'draw_point'

type DrawHandler = (event: any) => void;

export type Props = {
  drawing?: boolean,

  mode?: [DrawingMode] | ['direct_select', { featureId: string | number }],
  features?: Array<GeoJSON.Feature>,
  className?: string,

  children: (props: object) => React.ReactNode,

  // Options defined in https://github.com/mapbox/mapbox-gl-draw/blob/master/docs/API.md
  boxSelect?: boolean;
  clickBuffer?: number;
  controls?: Partial<{
    point: boolean;
    line_string: boolean;
    polygon: boolean;
    trash: boolean;
    combine_features: boolean;
    uncombine_features: boolean;
  }>;
  default_mode?: DrawingMode;
  displayControlsDefault?: boolean;
  keybindings?: boolean;
  position?: 'bottom-left' | 'bottom-right' | 'top-left' | 'top-right';
  touchBuffer?: number;
  touchEnabled?: boolean;
  userProperties?: boolean;
  styles?: object[];

  // Callbacks
  onActionable?: DrawHandler;
  onCombine?: DrawHandler;
  onCreate?: DrawHandler;
  onDelete?: DrawHandler;
  onModeChange?: DrawHandler;
  onRender?: DrawHandler;
  onSelectionChange?: DrawHandler;
  onUncombine?: DrawHandler;
  onUpdate?: DrawHandler;
}

type DrawControl = Control & {
  add: (geojson: GeoJSON.Feature) => void,
  changeMode: (mode: DrawingMode, options?: object) => void,
  deleteAll: () => void,
}

const featureIds = (features?: Array<GeoJSON.Feature>): Array<string> =>
  sortedUniq(map(features, 'id') as Array<string>)

export default class MapDraw extends PureComponent<Props> {
  map?: mapboxgl.Map
  draw?: DrawControl
  registeredCallbacks = new Set<string>()

  // tslint:disable-next-line: no-shadowed-variable
  setupMap = (map: mapboxgl.Map) => {
    this.map = map

    if (this.props.drawing) {
      this.initializeDraw()
    }
  }

  initializeDraw = () => {
    if (this.map) {
      this.draw = new MapboxDraw(this.props)

      this.map.addControl(this.draw!)

      this.map.on('draw.actionable', this.props.onActionable || noop)
      this.map.on('draw.combine', this.props.onCombine || noop)
      this.map.on('draw.create', this.props.onCreate || noop)
      this.map.on('draw.delete', this.props.onDelete || noop)
      this.map.on('draw.modechange', this.onModeChange)
      this.map.on('draw.render', this.props.onRender || noop)
      this.map.on('draw.selectionchange', this.props.onSelectionChange || noop)
      this.map.on('draw.uncombine', this.props.onUncombine || noop)
      this.map.on('draw.update', this.props.onUpdate || noop)

      this.updateDraw({})
    }
  }

  removeDraw() {
    if (this.map && this.draw) {
      this.map.removeControl(this.draw)
    }
  }

  updateDraw = (prevProps: Partial<Props>) => {
    if (this.map && !this.draw && this.props.drawing) {
      this.initializeDraw()
    }

    if (!this.map || !this.draw) {
      return;
    }

    if (!isEqual(featureIds(this.props.features), featureIds(prevProps.features))) {
      this.draw.deleteAll()
      if (this.props.features) {
        this.props.features.forEach((feature) => this.draw!.add(feature))
      }
    }

    if (this.props.mode && !isEqual(this.props.mode, prevProps.mode)) {
      this.draw.changeMode(this.props.mode[0], this.props.mode[1])
    }
  }

  componentWillUnmount() {
    this.removeDraw()
  }

  componentDidUpdate = (prevProps: Props) => {
    this.updateDraw(prevProps)
  }

  onModeChange = (event: { mode: DrawingMode }) => {
    // Reset mode change if manual mode set.
    this.updateDraw({
      ...this.props,
      mode: [event.mode],
    })
    if (this.props.onModeChange) {
      this.props.onModeChange(event)
    }
  }

  render() {
    return (
      <div className={this.classes()} >
        {this.props.children(this.mapProps())}
      </div >
    )
  }

  classes = () => classNames(this.props.className, {
    'mapbox-gl--drawing':  this.props.drawing,
  })

  mapProps = () =>
    this.props.drawing
      ? {
        setMap: this.setupMap,
        mapProps: {
          updateMapStyle: false,
          dragPan: false,
          onNativeClick: undefined,
        },
      } : {
        setMap: this.setupMap,
      }
}

A lot of this is still relatively raw, but the usage is intended to something like this:

<MapDraw drawing features=[someFeature] mode=['direct_select', { featureId: string | number }]>
  {mapProps => <InteractiveMap mapStyle={mapStyle} {...viewport} {...mapProps} />}
</MapDraw>

Users would toggle drawing/features and mode props according to whether they're making use of draw at this time or not and MapDraw is responsible for providing props to forward to InteractiveMap.

---

There's a lot of issues around this subject (#725, #450, #328, #197). At this point, I am pretty convinced that this is the smallest change needed to bring value to a lot of users - one prop used internally by other libraries doesn't seem like that bad of an overhead..

[Pessimistress]

So you're trying to use this new prop to block InteractiveMap from calling setStyle? What is the mapStyle that you are passing to the component then?

[macobo]

The mapStyle for us is a dynamically calculated style object following the style spec. It contains various things like filters, dynamic colors and all sort of things that can change due to user input in a large react/redux app outside of the map (e.g. interacting with a custom legend, navigation, etc).

Even while mapStyle object being passed doesn't change, interactivemap might rerender for other reasons. This can happen since for example the user changes their viewport via mouse or any other prop passed to InteractiveMap changes. When a rerender happens, this._map.setStyle is called and the two libraries will run into trashing conflicts (again, see #450).

[Pessimistress]

Even while mapStyle object being passed doesn't change, interactivemap might rerender for other reasons. ... When a rerender happens, this._map.setStyle is called

That should not be the case. https://github.com/uber/react-map-gl/blob/5.0-release/src/components/static-map.js#L202 prevents setStyle from being called if the prop does not change. So your mapStyle does change over time. Are you able to hold on to the original style during editing?

[macobo]

Note that the code you're linking is using reference equality, not actually comparing the contents of the styles.

But let's assume we were able to freeze the mapStyle - we would also need to build special logic at app-level to avoid e.g. viewport from changing, dragging and panning causing issues with moving vertexes on the map.

To take a step back; What's the main concern with this PR? What are concrete actions we can take to resolve them?

I get the vibe that you're assuming we're "misusing the library" and this review is more of a debugging session than a code review. If that's the case I'm not sure if I'm able to convince you otherwise and think that trying to do so is a lot wasteful effort for both of us.

I'm sure we're both out for the same thing here - make this library even better and provide useful tools. :)

[Pessimistress]

Note that the code you're linking is using reference equality, not actually comparing the contents of the styles.

Yes, and that is consistent with React's model as of what should trigger a rerender.

we would also need to build special logic at app-level to avoid e.g. viewport from changing, dragging and panning causing issues with moving vertexes on the map.

Doesn't dragPan: false or even onViewportChange: null already do that for you?

I'm sorry if I sound difficult here, all I'm trying to do is to understand why a new prop is needed. As far as I can see, if the mapStyle prop remains the same (shallowly), any other prop change should not result in setStyle being called. If that is the case for you, then there is a bug somewhere, and we should address it instead.

Ultimately, the purpose of this React wrapper is to ensure that component props and the rendered outcome are synchronized. May I propose, if I understand your use case correctly, that we add an initialMapStyle prop which is only used during the initialization of the map and ignored in subsequent updates?

[macobo]

Thanks for clarifying intentions! :)

I tested out removing prop and setting onViewportChange to undefined (good tip!) - this avoids the most common scenario for flicker, but we're still consistently calling staticMap.componentDidUpdate, causing flickering to occur as draw and react-map-gl fight it out.

I'm at this point not sure if it's the referential equality of mapStyle or something else causing the needless update. We're using reselect so that would eliminate the most common issue. I'll take a look at this in depth in the morning.

However, I'd argue that:

• react itself also provides componentShouldUpdate hook for advanced cases.
• React also degrades gracefully if you do too many updates - nothing besides peformance suffers that badly.
• I believe that mapbox-gl also does style diffing internally - even if static-map called setStyle too often, correctness wouldn't suffer, only performance due to cost of the diff.

Ultimately, the purpose of this React wrapper is to ensure that component props and the rendered outcome are synchronized. May I propose, if I understand your use case correctly, that we add an initialMapStyle prop which is only used during the initialization of the map and ignored in subsequent updates?

This sadly doesn't work. The usecase where we introduced this change + MapDraw is a multi-step interactive process which goes something like this:

1. User gets shown fields on a map (with search, etc all interacting with the map styles)
2. They can pick fields to add to their account
3. Optionally, they can edit the shape of these fields before adding it to their account
4. Eventually, users will also be able to combine/uncombine the objects on the map

We have ~3 other places in the application where we'd like to introduce drawing, and they all have similar tricky requirements - initialMapStyle would take away all benefits we're currently gaining from react-map-gl.

Some extra context: we've converted the majority of our application to react-map-gl over the past 7 months. This is ~10k lines of production code (though this includes tests) and ~15 different maps used in various ways through our map. Drawing is the last holdout for google maps currently.

---

I'm sure most applications are like ours and do some needless re-renders making some sort of escape hatch needed.

That said, will take a look at it again tomorrow if we can side-step this prop somehow all-together and perhaps provide a repro case.

[Pessimistress]

Thank you for the context. Assuming mapStyle is the only prop changing after you set onViewportChange to undefined - could it work if we skip updating mapStyle change whenever it's falsy, i.e. instead of <InteractiveMap {...props} updateMapStyle={false}> you use <InteractiveMap {...props} mapStyle={null}>?

[macobo]

Yes, that would work as well. :)

[Pessimistress]

@macobo Do you have time to update this PR? Do you want me to take over?

[Pessimistress]

Supported via mapStyle={null} instead.