Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: react-map-gl v7.0 #1646

Closed
Pessimistress opened this issue Dec 23, 2021 · 25 comments
Closed

RFC: react-map-gl v7.0 #1646

Pessimistress opened this issue Dec 23, 2021 · 25 comments
Assignees

Comments

@Pessimistress
Copy link
Collaborator

Pessimistress commented Dec 23, 2021

Background

react-map-gl was originally created by Uber's visualization team to work with deck.gl. Aside from providing a React-friendly wrapper for Mapbox GL JS, it currently also addresses use cases including:

  • Provide a base map layer for other visualization overlays, most extensively used in deck.gl and kepler.gl.
  • A source of React map components without mapbox-gl dependency, including Marker, Popup and NavigationControl

The project was transferred to the Urban Computing Foundation in April 2020.

Issues with the current code base

Diverged dependencies

react-map-gl currently have two releases in production:

  • v5.3 supports mapbox-gl v1 and maplibre-gl
  • v6.1 supports mapbox-gl v2

The divergence happened when Mapbox changed its license, see #1380. It has become increasingly difficult to maintain as Maplibre and Mapbox start making more aggressive breaking changes.

Override of the native event handling

react-map-gl disables mapbox-gl's native event handling and implements its own. This is arguably the one most controversial design decision of this project, dated back to before the current maintainers were involved. Lengthy discussions have been had (e.g. #569, mapbox/mapbox-gl-js#3746, #725) and continues to generate hard-to-resolve feature requests (#574, #1640, #1353, #775) and performance issues (#1151, #909).

Despite major efforts to improve the usability of the custom controller, it still has many open issues. The code base is quite complex already, with few developers able to navigate and contribute. Considering the complexity of the task, it is unlikely for the current release to support Mapbox's non-WebMercator projection feature in its current state.

Proposal

This RFC is gathering feedback for the following major breaking changes proposed for react-map-gl v7.

  • Make mapbox-gl a peer dependency so that the library is impartial about the version/fork used.
  • Drop the usage of mjolnir.js' EventManager and subsequently MapController altogether. Instead, we will pursue exposing mapbox-gl's native event system and attempt to hijack the camera change under the hood.
  • This will allow users to use the latest rendering and interactive options when they are released by Mapbox.
  • 3rd-party Mapbox addons will be supported by default.
  • We can add a stateful version of MapGL that renders fully asynchronously without any performance impact.
  • Remove transition interpolator classes and use native methods such as Map.flyTo and Map.easeTo.
  • Custom MapController classes will stop working.
  • Remove the overlay classes.
  • Components such as Marker, Popup etc. will no longer be mapbox-independent. We will use React portal to render them into mapbox-gl's control container, instead of floating outside of the map container. deck.gl applications that render these components as children of DeckGL will stop working. I propose we move some of the current code to a new React component library that is independent from mapbox-gl.
  • Rewrite the code base in TypeScript.

The goal is to have a code base that is less demanding to maintain, and more focused on the most common use cases. To achieve this, we have to drop some niche use cases and/or move their support to other projects.

@Pessimistress
Copy link
Collaborator Author

Pessimistress commented Dec 23, 2021

@Pessimistress Pessimistress pinned this issue Dec 23, 2021
@ibgreen
Copy link
Contributor

ibgreen commented Dec 23, 2021

This sounds like the right move to secure the future of this framework. This should make the code base considerably simpler, and both reduce the burden on current maintainer and make the react-map-gl code more accessible to new maintainers as well as the community.

Is it possible to quickly spin up a prototype? Being able to see an alpha release of the new approach work in a major app like kepler.gl would be a big confidence builder.

I propose we move some of the current code to a new React component library that is independent from mapbox-gl.

nebula.gl also has some at least superficially similar overlay components. Perhaps an alignment opportunity?

@Pessimistress
Copy link
Collaborator Author

I have pushed a proof of concept here: https://github.com/visgl/react-map-gl/tree/prototype

Tested with the following use cases:

  • Controlled component
function App() {
  const [viewState, setViewState] = useState({
    latitude: 40,
    longitude: -100,
    zoom: 3.5,
    bearing: 0,
    pitch: 0
  });

  return <Map
    {...viewState}
    onMove={({viewState}) => setViewState(viewState)}
    mapStyle="mapbox://styles/mapbox/dark-v9"
  />;
}
  • Stateful component
function App() {
  return <Map
    initOptions={{
      latitude: 40,
      longitude: -100,
      zoom: 3.5,
      bearing: 0,
      pitch: 0
    }}
    mapStyle="mapbox://styles/mapbox/dark-v9"
  />;
}
  • External controller
function App() {
  return <DeckGL
    initialViewState={{
      latitude: 40,
      longitude: -100,
      zoom: 3.5,
      bearing: 0,
      pitch: 0
    }}
    controller
  >
    <Map mapStyle="mapbox://styles/mapbox/dark-v9" />
  </DeckGL>;
}

@alasarr
Copy link

alasarr commented Dec 28, 2021

Thanks a lot for the huge work here @Pessimistress.

It totally makes sense to me, vis.gl should continue being basemap agnostic. It reduces complexity, it's easy for maintainers, and brings the possibility of getting the features implemented by the new providers.

Definitely, a better strategy for the current situation with mapbox-gl.

@VictorVelarde we should try this prototype after the Christmas holidays to see how it fits in CARTO products (mainly CARTO for React and Builder)

cc: @zbigg @padawannn @bbecquet @felixpalmer @Josmorsot

@Pessimistress
Copy link
Collaborator Author

You can try the work-in-progress release with react-map-gl@beta.

Docs: https://github.com/visgl/react-map-gl/tree/7.0-dev/docs/api-reference

@timsluis
Copy link

timsluis commented Jan 4, 2022

Thanks a lot @Pessimistress. Tried it yesterday and it is already working like a charm! It's faster, it is (way) smaller and less lines of codes are needed for implementing it in our own codebase. 👏

@felixpalmer
Copy link

Great to see this. Thanks for all the effort @Pessimistress.

I have two questions regarding the reversal of control flow (dropping mjolnir):

@Pessimistress
Copy link
Collaborator Author

Pessimistress commented Jan 4, 2022

@felixpalmer You can still use DeckGL to control the map, which is the main use case for nebula and all deck.gl examples. It's just that react-map-gl no longer implements a controller.

@timsluis
Copy link

timsluis commented Jan 6, 2022

Hi @Pessimistress, I have been testing v7 quite extensively. How do you want to receive the feedback?

@Pessimistress
Copy link
Collaborator Author

@timsluis That's what this thread is for.

@timsluis
Copy link

timsluis commented Jan 8, 2022

Three things I noticed:

  • The customAttributions prop of the AttributionControl component should be called customAttribution.
  • If you have React Strict Mode turned on, the Popup component will create two popup objects and adds them both to the map. Only one will be removed once when you unmount the Popup component. This happens because Strict Mode intentionally double-invokes the State updater functions (the first argument to setState). This is where the popup gets created and added to the map object.
  • Also, the content of a popup is rendered into the React Portal after the Popup has been added to the Mapbox map. This causes the popup to be placed partly outside the viewport (if opened on the edge of the screen), since Mapbox isn't able to position it correctly. After a map interaction the Popup is positioned correctly again.

@slaymance
Copy link
Contributor

slaymance commented Jan 14, 2022

Fully support this direction for the project!

With the move of mapbox-gl to be a peer dependency, has there been any discussion around supporting user-specified map libraries (specifically, maplibre-gl as opposed to mapbox)?

The current practice of using a mapbox-gl fork works great for applications, but makes things problematic for users wishing to use react-map-gl with something like maplibre-gl in a published package since the publisher doesn't control the end-user's bundler configuration.

I've thought a bit about this before and considered something like dynamic imports that landed in ES2020:

// src/utils/mapboxgl.js
export default (await import(config.MAP_LIBRARY ?? 'mapbox-gl')).default;

I'm not sure what would be a good API to allow users to set their preferred map library. Any thoughts on this idea?

@Pessimistress
Copy link
Collaborator Author

@brandonjpierce
Copy link

Awesome changes, the transition from v6 to v7 was quite seamless for my team. Only issue we are seeing so far is the following:

  • We are using deckgl v8.6.8
  • We are using react-map-gl v7.0.0-beta.1
  • We are controlling the viewport with React useState

TL;DR:

const [viewState, setViewState] = useState(initialViewport);

const onViewStateChange = useCallback((e: any) => {
  setViewState(e.viewState);
}, []);

return (
  <DeckGL
    viewState={viewState}
    onViewStateChange={onViewStateChange}
  >
    <Map>
      <ScaleControl />
      <NavigationControl />
    </Map>
  </DeckGL>
);

The NavigationControl cannot be interacted with via mouse events and if you tab to the controls and active via spacebar / enter it desyncs deckgl from mapbox view state.

@Pessimistress
Copy link
Collaborator Author

@brandonjpierce there is a section in the RFC that discusses deck.gl support. TLDR is that v7 controls will no longer work with the DeckGL component.

@brandonjpierce
Copy link

Apologies, I must have missed that. Thanks for the info!

@bbecquet
Copy link

bbecquet commented Feb 4, 2022

Hi!
We are testing v7 on CARTO builder platform, everything is going smoothly for now. We use the feature to import maplibre-gl in place of mapbox-gl and it worked like a charm (now I'll cross my fingers that MapLibre doesn't divert too much from MapBox in the near future).
More generally the simpler API is really neat, and some dropped part like mjolnir will probably bring us to simplify parts of our own code, which is great.
Can't wait to make the switch!

Thanks for the awesome work 👍

@Pessimistress
Copy link
Collaborator Author

Thanks to everyone on this thread for your feedback! 7.0.0 has been published. Please open new issues for bugs and feature requests.

@sangdth
Copy link

sangdth commented Feb 5, 2022

Thanks for the awesome works, @Pessimistress (and other collaborators of course) I haven't tried anything complex but overall I can see the significant performance improved!

🙏🏻 ❤️

@dreamorosi
Copy link
Contributor

dreamorosi commented Feb 6, 2022

Thanks for the amazing work @Pessimistress, have been testing this with maplibre-gl and love the fact that aliasing is not necessary anymore; also been noticing performance improvements which is always nice.

@victcebesp
Copy link

victcebesp commented Feb 21, 2022

Hello guys! I started using this awesome framework a couple of weeks ago on a legacy project. They started using MapBox and now wanted to change to MapLibre. However, I tried following the tutorial on the react-map-gl website on a brand-new create-react-app project and it is not working.

The following error keeps appearing:
ERROR in ./node_modules/react-map-gl/dist/esm/components/map.js 52:30-49 Module not found: Error: Can't resolve 'mapbox-gl' in '/mnt/e/Bettermaps/maplibre-test/node_modules/react-map-gl/dist/esm/components'

It seems like it is still looking for mapbox-gl even though as explained on the tutorial, I am passing the maplibre library to the Map using the prop mapLib...

Any clues?

Thank you very much!

(I have not added any more detail about the code as it is failing on a brand-new create-react-app project)

Here you have the package.json in case it helps:

{
  "name": "maplibre-test",
  "version": "0.1.0",
  "private": true,
  "dependencies": {
    "@testing-library/jest-dom": "^5.16.2",
    "@testing-library/react": "^12.1.3",
    "@testing-library/user-event": "^13.5.0",
    "maplibre-gl": "^2.1.6",
    "react": "^17.0.2",
    "react-dom": "^17.0.2",
    "react-map-gl": "^7.0.6",
    "react-scripts": "5.0.0",
    "web-vitals": "^2.1.4"
  },
  "scripts": {
    "start": "react-scripts start",
    "build": "react-scripts build",
    "test": "react-scripts test",
    "eject": "react-scripts eject"
  },
  "eslintConfig": {
    "extends": [
      "react-app",
      "react-app/jest"
    ]
  },
  "browserslist": {
    "production": [
      ">0.2%",
      "not dead",
      "not op_mini all"
    ],
    "development": [
      "last 1 chrome version",
      "last 1 firefox version",
      "last 1 safari version"
    ]
  }
}

@sangdth
Copy link

sangdth commented Feb 21, 2022

Have you tried to install mapbox-gl? I don't need to provide any maplibre in my app.

@victcebesp
Copy link

Thank you very much for your answer!

Have you tried to install mapbox-gl? I don't need to provide any maplibre in my app.

Yeah, it looks like for some reason the mapbox-gl library was removed from the node_modules (it was installed when I installed react-map-gl. But Isn't the whole point about not using mapbox-gl?

In addition, even though now I am not getting that error, I am getting the following error:

Error: NetworkError when attempting to fetch resource.

I think that this is related to the mapbox token as now I am using a mapbox style but I am passing the token to the initialized map as you can see in the following code snippet:

import "./App.css";
import Map from "react-map-gl";
import maplibregl from "maplibre-gl";
import "maplibre-gl/dist/maplibre-gl.css";

function App() {
  return (
    <div className="App">
      <Map
        mapboxAccessToken="MY_TOKEN_HERE"
        mapLib={maplibregl}
        initialViewState={{
          longitude: -100,
          latitude: 40,
          zoom: 3.5,
        }}
        mapStyle="mapbox://styles/mapbox/streets-v9"
      />
    </div>
  );
}

export default App;

Again, here is my package.json:

{
  "name": "maplibre-test",
  "version": "0.1.0",
  "private": true,
  "dependencies": {
    "@testing-library/jest-dom": "^5.16.2",
    "@testing-library/react": "^12.1.3",
    "@testing-library/user-event": "^13.5.0",
    "maplibre-gl": "^2.1.6",
    "react": "^17.0.2",
    "react-dom": "^17.0.2",
    "react-map-gl": "^7.0.6",
    "react-scripts": "5.0.0",
    "web-vitals": "^2.1.4"
  },
  "scripts": {
    "start": "react-scripts start",
    "build": "react-scripts build",
    "test": "react-scripts test",
    "eject": "react-scripts eject"
  },
  "eslintConfig": {
    "extends": [
      "react-app",
      "react-app/jest"
    ]
  },
  "browserslist": {
    "production": [
      ">0.2%",
      "not dead",
      "not op_mini all"
    ],
    "development": [
      "last 1 chrome version",
      "last 1 firefox version",
      "last 1 safari version"
    ]
  }
}

Thank you very much!

@victcebesp
Copy link

Is there any open project on which they are using maplibre with react-map-gl in order to check what I am doing wrong? Thanks guys!

@dreamorosi
Copy link
Contributor

@victcebesp, please consider opening a Q&A discussion or a new issue/bug report instead of commenting here. The discussion has been closed a long while ago and a lot of people are subscribed to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests