React-mapbox-gl: About ReactMapboxGl design: Map properties and options

[lamuertepeluda]

Hi again,

I was looking at the source code of the default class (the map) and I think it should be designed as Factory function/Higher Order Component, since there are some properties that cannot be changed after the instantiation (meaning their change has no effect).

For instance, setting interactive={false}then changing it lately to true, has no effect. This is mostly because it is an option of the underlying map which is passed to the MapBox Map constructor, rather than a property that can be set or read using setters ad getters.

Because of this, I think that properties that cannot be changed by the parent components should be passed to a factory function, e.g. something like this (pseudocode)

const ReactMapboxGlFactory (options) => {
   //use options for instantiating the map instance
   const opts = {...defaultOptions, ...options};

   /*...other initializations */
   return class class ReactMapboxGl extends React.Component{
       //options are passed to the MapBoxGl constructor
       const map = new MapboxGl.Map(opts);
       //properties are mapped to the setters/getters of the returned component
    }
}

Otherwise we could hold the current structure, but we'd have to find a way of updating the map instance when changing the options, which is trickiest, but would require cloning the current MapBoxGl instance and re-instantiating it with the updated constructor options.

What do you think about it?

[alex3165]

Hey @lamuertepeluda thanks for the issue, that is a true but what is the benefit of doing so ? properties that doesn't change don't affect anything at re-rendering, we can just ignore them during the entire component life cycle.

[lamuertepeluda]

Uhm, for instance I was trying to make the map not interactive (set to false) until a certain initial ease was made, and I noticed this when I was setting interactive again. That's what you expect from a react component (unless we put at least some caveats in the docs), that is updating props should reflect in the children...

[z0d14c]

+1 for interactive not being able to change 😩

[kitten]

It might be worth considering this, although it's a breaking change. There's a lot here that can be done, in terms of improving the API, and making the instantiation of the config part of a factory would indicate that it's not something that can change, thus getting rid of a lot of unexpected short comings

[alex3165]

Ok I guess we can give it a go.

[lamuertepeluda]

I know it is a breaking change, but what about dropping the context API/setState API in favor of a redux-managed state that allow pass the map mutations as props?

Potential advantages of using redux:

• avoid component state mismatches (e.g. setState being called after unmount)
• have children as connected components
• drop context api as warmly suggested here
• easier debug of map state with redux dev tools
• have a state provider HOC (e.g. withMapBox(...) ) that allow programmers to connect external components to the map state, for instance for displaying map stats, or implement custom controllers
• optionally persist the state (e.g. with redux-persist)

[cpsubrian]

Thanks @lamuertepeluda, you got me thinking. I'm working on a spin-off of this module internally that is hugely inspired by react-mapboxgl, but tries to stick as close to the raw mapbox-gl api as possible. I want to play with this approach.

[kitten]

@lamuertepeluda just as a quick note, although I'm not working actively on this project anymore: Dropping the context based API won't have any major outcome or benefit. While that doc is indeed saying that context is unstable, it is used throughout the community for use-cases just like this one.

We have state that is managed by Mapbox GL and we'd like to just share it. The "state" is stored inside their API and we need to interact with it. The most logical solution is propogating the map through the context API, which is not a problem, and avoids all of context's caveats, since the map reference stays equal after initialisation.

Switching to Redux would only bloat this library even more, and wouldn't do us any good, as it only moves a reference to already existing "state" to another state management location.

The way to go in my opinion is to separate the mapbox initialisation from the React Mapbox GL root component, so that modifications to it don't matter anymore and turn this library into a leaner layer on top of it.

To address some of your concerns separately:

avoid component state mismatches (e.g. setState being called after unmount)

This is a pure logic problem in the current implementation and wouldn't be solved by moving to Redux. The callbacks passed to the map container do have to lead somewhere, and need to be cleaned up properly.

have children as connected components

We're achieving the same behaviour using our own context.

easier debug of map state with redux dev tools

It might seem like a good idea, but we don't have a lot of interactions and our own state that are valuable. This would only lead to an additional bridge:

React Mapbox GL -> Redux -> Map Init -> Internal Callbacks -> Actions with mirrored state in Redux -> React Mapbox GL Sideeffects

This pattern introduces complexity that we don't need (we don't need side-effects triggering any other effects) and duplicates the map state that is already managed by Mapbox GL.

have a state provider HOC (e.g. withMapBox(...) ) that allow programmers to connect external components to the map state, for instance for displaying map stats, or implement custom controllers

Can easily be introduced to the current context API. The issue just hasn't come up afaik.

optionally persist the state (e.g. with redux-persist)

What we'd persist is "meta state" like map location / tilt / etc, that is indeed managed duplicated in the root component, but can easily be managed externally by hooking into the context / preserving initial props to React Mapbox GL.

Another issue is that once we introduce Redux we also have to manage multiple maps, so ouch to that one.

The key ideas to solving potentially all problems here are:

• Let Mapbox GL keep its state
• Make React Mapbox GL's Map component leaner over time
• React to changes and don't duplicate state, while offering props to proxy to Mapbox GL's state

[alex3165]

https://github.com/alex3165/react-mapbox-gl/pull/239 Make the Map a factory function which return a React component, will be merged in v2.0.0

[alex3165]

I will close this issue as it is been resolved with the version 2.0.0. Please use https://github.com/alex3165/react-mapbox-gl/issues/237 to be updated on the state of 2.0.0.