Mapbox v10 checklist

[naftalibeder]

EDIT: This is up at https://github.com/users/naftalibeder/projects/2.

---

(Moving this from #1838, cc @mfazekas.)

It would be great to came up with v10 specific API, that gives use flexible way to handle camera animations, and also Viewport api

I would love to have a go (in a different PR!) at creating a different API for the camera. That makes me wonder, however, if we should turn main into a dedicated pre-v10 branch, and have a separate v10 branch that will eventually be a breaking change. Or, have main (v10) and legacy (<10) branches.

Otherwise, I think we're going to water down changes, or add unnecessary complexity to the API, to keep backward compatibility.

What does anyone think about v10 being a breaking / non-breaking update?

[mfazekas]

@naftalibeder can't we just create a new component for the new api, if the api is much different - which I'd say it should be. If there are slight changes then some compatibility breaks here and there is ok.

• having mostly compatible api with v8.* means it's easy to migrate code.
• I'd prefer not creating a new branch as that complicates development. I'd prefer some code duplication to parallel development.

[naftalibeder]

Absolutely - so are you saying that we create v10-specific components only when we'd like to create a breaking change, and otherwise all changes happen in the root components directory?

I see the value in that - we only need to overwrite certain components if/when we finally drop support for <v10.

The strongest counterargument is that if we branch, we can lock <v10 in a "stable" maintenance-mode state, and be very free with v10 changes. But no objection if you feel strongly about not doing that.

[mfazekas]

To clarify in the coming months, I'd prefer keeping MapLibre and classic mapbox working, and also switching with v10 implementation, with no or very minimal code changes, so migration to rnmapbox/maps v10 is easier.
I don't mind those:

• new v10 only functionality via new modes, functions, properties, components.

Later we can fork a maplibre version and remove non v10 stuff from the main branch. For now I'd like to avoid that fork unless there are concrete strong arguments for it. We can also create a new branch for experimenting with say new Camera components api unit it's finalised.

[dorthwein]

Is there a checklist for whats left to do to get v10 stable - i.e. core functionality of camera movements and loading lays on the map?

[naftalibeder]

@dorthwein Not that I know about, but it would certainly be a good idea to have one! I'll update this discussion and we can get a list going, and update it as we discover more things. This would also help people claim functionality to work on, so we don't accidentally have overlapping PRs opening up.

[mfazekas]

I think we can use GitHub Issues for that list.

[Gnative]

Could use Github projects (boards) to flesh out the list - turning them into issues as needed ?

https://github.com/features/issues

[naftalibeder]

Great idea - how's this? https://github.com/users/naftalibeder/projects/2/settings

Let me know if you want write access!

[naftalibeder]

@mfazekas and anyone else:

Does anyone have an opinion about possibly deprecating the imperative methods on Camera? IMO the main point of this library is to take the imperative native SDK interfaces and plug them into the state-based React system.

Calling e.g. setCamera() explicitly detaches native state from JavaScript state, which is (I believe) a disconnect that introduces a lot of bugs - in React, a no-op (or barely-op) prop change should not cause the content onscreen to change meaningfully - which is what happens if you point the camera at a state coordinate, then call setCamera() to point somewhere else, and then the coordinate changes by even 0.00001.

It would also just simplify the Camera interface a lot to make it purely reactive.

[mfazekas]

@naftalibeder I think the way we added Camera as a state is not complete, therefore the setCamera is necessary in my opinion.

The issue is that camera is not a controller component.

For example
<Camera centerCoordinate={[-74.00597, 40.71427]} zoomLevel={14} />
doesn't mean anything about the current state of the camera. It means that at one point we've started a transition to zoomLevel=14 and coordinates [-74.00597, 40.71427]. User might have changed zoom level, or center since then.

We have a workaround with triggerKey, but that is a workaround and I prefer the imperative methods over that.

IMO the main point of this library is to take the imperative native SDK interfaces and plug them into the state-based React system.

I think the main point of this library to enable use of mapbox libraries via react-native. If we can make good and complete state based components that's great. If not, I'd prefer a lower level procedural interface to a state based components that doesn't supports all usescase. Of course the ideal if we find good state based interfaces.

[naftalibeder]

This is all fair. A bit of a lengthy response below, but I promise it's well-intentioned. :)

My understanding of the ideal usage would look more like this, with the user's own component being the controller:

const MapController = () => {
  const camera = useRef();
  const [centerCoordinates, setCenterCoordinates] = useState([-74.00597, 40.71427]);

  useEffect(() => {
    setTimeout(() => {
      camera.setCamera({
        centerCoordinate: [0, 0],
        zoomLevel: 16,
      }
    }, 1000);
  }, []);

  <MapView>
    <Camera
      ref={camera}
      centerCoordinate={centerCoordinate}
      zoomLevel={14}
    />
  </MapView>
}

After 1 second, the camera shifts to [0, 0], and now MapController has invalid state - all it would take is a tiny change in centerCoordinates to make the camera jump back to its original position. It's true that Camera.js itself doesn't have any state that can become invalid, but the imperative method allows a native call that is disconnected from the controller-state props that are passed in.

If the camera's interface was more opinionated, then the user would be encouraged to update their own state. As it is now, reports like this are challenging to debug, because the duration has potentially two sources of truth - state and method call.

The only time I've really seen imperative methods lead to good component design is for aspects of a component that aren't controllable with state, e.g. a scrollview offset (scrollview.setOffset(10)).

[mfazekas]

The only time I've really seen imperative methods lead to good component design is for aspects of a component that aren't controllable with state, e.g. a scrollview offset (scrollview.setOffset(10)).

Yes scroll view is an excellent example. ScrollView offset determines which part of document is visible in the current ScrollView. Just like camera settings determines which part of the map is visible in the MapView. And both scroll and camera can be changed by user interaction. Maybe it's disabled in your use case, but in general gestures can be used to change the camera by the user.

[naftalibeder]

Sure, however a scrollview can only be updated programmatically in one way, the imperative method. You wouldn't want a scrollview with an interface like this (or would you?):

const ScrollViewController = () => {
  const scrollview = useRef();
  const [offset, setOffset] = useState(10);

  useEffect(() => {
    setTimeout(() => {
      scrollview.setOffset(50);
    }, 1000);
  }, []);

  return (
    <ScrollView
      ref={scrollview}
      offset={offset}
    />
  )
}

[danidaryaweesh]

Anyone using V10 in a production system? Is it safe to use in production and have you noticed any performance improvements as Mapbox team promised in V10?

I would appreciate any feedback.

[dorthwein]

@naftalibeder - are y'all using v10 in production?

[naftalibeder]

We are not, but we’re working on a branch that uses v10, with the intention of bringing it to production when we feel it’s ready. (Right now there are too many things that are unimplemented or partly implemented.)

But to @danidaryaweesh ‘s question, so far it does seem more stable and faster, though I don’t have any numbers to back that up.

[tomskopek]

I've tested v10, not extensively. So far it seems to run a lot faster on our simulators 😄

From what I can tell, only two things are preventing us from using it (as of May 10, 2022):

1. textFont style on SymbolLayer does not seem to be working

2. We need a similar implementation of onRegionDidChange and onRegionDidChangeDebounceTime on MapView. Although we can probably implement this one manually using a combination of onCameraChanged and onMapIdle. (We do a fetch every time the map movement has settled)

@mfazekas @naftalibeder I'd love to contribute to the project. However, I don't know swift or kotlin. Do you think either of these tasks ☝️ are feasible "starter" tickets for someone to dip their toes in? ie. with a little bit of guidance do you think a beginner could take them on?

[mfazekas]

@tomskopek #1906 should fix textFont style values as it's an array of numbers. I'm considering going back to onRegionDidChange, but implementing debounce is definitely possible with the current onMapIdle callback.