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

Migrate to MapLibre #7015

Merged
merged 117 commits into from
Aug 8, 2024
Merged

Migrate to MapLibre #7015

merged 117 commits into from
Aug 8, 2024

Conversation

birkskyum
Copy link
Contributor

@birkskyum birkskyum commented Jun 5, 2024

This PR :

  • Swaps @plotly/mapbox-gl for maplibre-gl, as suggested here.
  • Removes the now-unused mapboxAccessToken from plotly.js public api (if needed, it's possible to provide a style api keys in the url as shown here)

I was asked to explain the advantages of using maplibre over mapbox v1 (like the plotly fork), or mapbox v2+, which are:

advantages over mapbox-gl v1 or a fork of it like @plotly/mapbox-gl

  • Full typescript and accurate documentation. The plotly fork of mapbox-gl v1 points to api docs for a much newer major version of mapbox-gl. The mapbox-gl v1 docs aren't online anymore.
  • The maintenance burden is moved to the MapLibre org. There is paid stable maintenance on the project, and also financial investments in new feature development, so PR's /issues won't be left for dead. All this is covered by the awesome MapLibre Sponsors who all benefit from the existence of MapLibre because it allow the cost to be shared instead of covering full maintenance/development of separate forks, and it keeps an industry wide compatibility. On that note, and it's entirely optional, but if you find that maplibre continue to bring Plotly value, you're of course also more than welcome to join the ranks and help financially sustain the non-profit or contribute PRs.
  • Lots of performance work has been done - notably microsoft has been helping out by profiling bing maps usage extensively on the millions of users there.
  • maplibre has since v3 supported webgl2 which almost all users will be using at this point, and in the increasingly rare case it's not supported there is still a webgl1 fallback in place.
  • there is a growing community around maplibre gl, we've e.g. seen a 100% increase in downloads (now ~400k/weekly) just over the past half year.
  • Monthly open (no registration) TSC for community feedback /discussions / group decisions.
  • many companies are helping out with new features related to their respective domain of expertise.
  • Continous support for emergent standards, like the new datasets from the Overture Maps Foundation as shown in this example.

here are contribution graphs, with the area approx. marked after the fork, to visually represent the scale of shared continuous investments:

@plotly/mapbox-gl ( 11 PR's )
Screenshot 2024-06-08 at 13 03 29

maplibre-gl ( 2000+ PR's)
Screenshot 2024-06-08 at 13 03 36

advantages over mapbox-gl v2+

  • maplibre-gl and mapbox-gl v1 are rendering libraries where mapbox-gl v2 is a rendering service. Both can be paired with data/styles from local files or services, but the difference is that by staying a renderer library, maplibre itself is free to use, and will run without an api key. This has multiple advantages:
    • allows rendering offline, on-prem
    • gives better startup performance, because the code doesn't "calling home" to check the api key billing status
    • It's more convenient for new users, not to have to signup for mapbox and get an api key.
  • The maplibre license permits removing the logo from lower left, in case plotly.js users rather see more of their plots
  • You're usecase require bundling the renderer into plotly, which you can certainly do with maplibre-gl, but there was a mention here that might not be possible with mapbox-gl v2.

closing #5578

@birkskyum
Copy link
Contributor Author

birkskyum commented Jun 5, 2024

Most/all examples run fine already in the npm run start browser. I haven't updated any snapshot yet, which will clear some more unit tests.

@archmoj archmoj marked this pull request as ready for review June 5, 2024 21:13
@archmoj archmoj added community community contribution status: reviewable labels Jun 5, 2024
@birkskyum
Copy link
Contributor Author

birkskyum commented Jun 6, 2024

Many things are working. Mainly wrestling some openmaptiles fonts at this point - missing fonts like the Arial MS one is breaking the baseline test suites. It's probably better to use the very complete Noto in that case.

@birkskyum
Copy link
Contributor Author

birkskyum commented Jun 6, 2024

the publish-dist suite gives errors because of the es5 compile target:

/home/circleci/plotly.js/dist/plotly-with-meta.js
  184628:3558  error  Parsing error: The keyword 'let' is reserved

/home/circleci/plotly.js/dist/plotly.js
  182907:3558  error  Parsing error: The keyword 'let' is reserved

Resolved by

webpack.config.js Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
lib/index-maplibre.js Outdated Show resolved Hide resolved
@archmoj archmoj removed this from the v3.0.0 milestone Jul 15, 2024
@archmoj
Copy link
Contributor

archmoj commented Jul 15, 2024

@birkskyum This clearly demonstrates the migration to maplibre in plotly.js v3 is possible.
FYI - I set the status to on hold for now as we may use your PR as a baseline to introduce new trace types e.g. scattermap, etc. that use maplibre for their rendering in a minor plotly.js version.

Thanks very much for the PR. 🏆

@archmoj
Copy link
Contributor

archmoj commented Jul 26, 2024

Hi @birkskyum,
I found a blocking bug on this branch as well as my PR related to displaying marker.symbol.
On this branch if I call the following the marker symbols won't render:

Plotly.newPlot(gd, {
    data: [{
        type: "scattermapbox",
        mode: "markers+text+lines",
        lon: [-75, -80, -50], 
        lat: [45, 20, -20],
        marker: {
            size:20, 
            symbol: ["bus", "harbor", "airport"]
        }
    }]
});

Could you possibly send a PR targeting #7060's branch?
Please note on that branch the scattermap should be used instead of scattermapbox.
Thank you!

@birkskyum
Copy link
Contributor Author

birkskyum commented Jul 26, 2024

Hi @birkskyum, I found a blocking bug on this branch as well as my PR related to displaying marker.symbol. On this branch if I call the following the marker symbols won't render:

Could you possibly send a PR targeting #7060's branch? Please note on that branch the scattermap should be used instead of scattermapbox. Thank you!

pr here:

@gvwilson
Copy link
Contributor

gvwilson commented Aug 8, 2024

see also #7060

@archmoj archmoj merged commit d09cdd4 into plotly:master Aug 8, 2024
1 check passed
@archmoj
Copy link
Contributor

archmoj commented Aug 8, 2024

Thanks @birkskyum 🏆
All your commits here are now used by merging #7060.

@ndrezn
Copy link
Member

ndrezn commented Aug 8, 2024

Seconding the thanks! @birkskyum as you know we're really grateful for your contribution and super excited about the move to MapLibre as the rendering service for Plotly map traces. Cheers!

@archmoj
Copy link
Contributor

archmoj commented Aug 8, 2024

@birkskyum For now these number of commits simply put you at #25 on the contributors list of all time!
Screenshot from 2024-08-08 10-29-50

And third in the last year!
See https://github.com/plotly/plotly.js/graphs/contributors?from=2023-08-08&to=2024-08-08&type=c

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

Successfully merging this pull request may close these issues.

Switch from Mapbox GL JS to MapLibre GL
5 participants