Migrate to MapLibre

[birkskyum]

trafficstars

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 )

maplibre-gl ( 2000+ PR's)

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]

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.

[birkskyum]

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]

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

• #7017
• #6909

[archmoj]

Could you please provide a fix so that the extra bold render on mapbox_fonts-supported-open-sans mock?

[birkskyum]

Could you please provide a fix so that the extra bold render on mapbox_fonts-supported-open-sans mock?

I can add the patch from the other PR that makes it appear, but as a regular weight, until a better fix comes along.

[archmoj]

Could you please provide a fix so that the extra bold render on mapbox_fonts-supported-open-sans mock?

I can add the patch from the other PR that makes it appear, but as a regular weight, until a better fix comes along.

That's perfect here.

[archmoj]

Now let's add 'mapbox_geojson-attributes' and 'mapbox_layers' to the blacklist here. https://github.com/plotly/plotly.js/blob/7c22d2f988446fabbdbfe3e304789e3045d72ecf/test/image/make_baseline.py#L97-L102 For now this would help we have a successful baseline creation and then the image comparison test would start on CircleCi to highlight the diffs.

[birkskyum]

Added those to blacklist now

[archmoj]

You also need to fix rendering of mapbox_fonts-supported-open-sans. Thanks!

[birkskyum]

You also need to fix rendering of mapbox_fonts-supported-open-sans. Thanks!

I fixed that now, and i also fixed the mapbox_geojson-attributes by pointing Open Sans Regular,Arial Unicode MS Regular to just Klokantech Noto Sans Regular which is sufficiently complete as mentioned here, so i took it off the blacklist

[birkskyum]

I removed all blacklists but the mapbox_layers, because they don't need the mapbox api key anymore

mapbox_layers need some contour data similar to here, but nothing indicate it would fail if it got the data.

[archmoj]

Almost there. Let's also blacklist these mocks for now. Later we will investigate why they are failing.

'plot_types', 
'trace_metatext', 
'zz-gl3d_surface_small_timerange',

[birkskyum]

Neither of those 3 are failing for me, are you on the latest commit?

[birkskyum]

[archmoj]

OK. Let's watch which one fails on the CircleCi now.

[birkskyum]

Added back these in the blacklist:

blacklist = [ 'mapbox_density0-legend', 'mapbox_osm-style', 'mapbox_stamen-style', 'mapbox_custom-style', # Figure out why needed this in https://github.com/plotly/plotly.js/pull/6610 'mapbox_layers' ]

can always be taken out later on

[archmoj]

No! By looking at failures of different parallel runs I think the blacklist should include the following now:

[
  'mapbox_stamen-style', 
  
  // TODO: figure out why?
  'plot_types', 
  'trace_metatext', 
  'zz-gl3d_surface_small_timerange',
]

[birkskyum]

Also added mapbox_layers, as it'll likely break until it has the contour data

[birkskyum]

it's like this now: blacklist = [

'mapbox_stamen-style', 'mapbox_layers', 'plot_types', 'trace_metatext', 'zz-gl3d_surface_small_timerange', ]

[archmoj]

Excellent. Now please download the baselines.tar from the artifatcs tab of test-baseline test here: https://app.circleci.com/pipelines/github/plotly/plotly.js/10578/workflows/3582843f-823a-45b2-9d65-3eff4281d1be/jobs/232240/artifacts

And extract the files and replace the PNGs inside test/image/baseline folder; then commit and push.

[birkskyum]

They are updated now, and the source-syntax should pass as well.

[archmoj]

Excellent. It looks like parts of the changes to the baselines in 7cd3177 are coming from the changes you made to the test/image/mocks. Now let's investigate what would happen if you revert those mocks?

[birkskyum]

They are reverted now, but it won't be pretty as the old mocks use the mapbox style (style api service) keywords like "outdoors", "satellite" and "basic" that doesn't mean anything anymore.. so we'll have to define those keywords to some meaningful non-mapbox styles, or redirect them to the keywords that still function like the carto-positron etc..

https://github.com/plotly/plotly.js/pull/7015/files#diff-4c999e79e2db6d26fdc437b32284ecb9cf566c275e50c56ad8946b1405e1b27fL176

[archmoj]

They are reverted now, but it won't be pretty as those mocks use the hardcoded mapbox style keywords like "outdoors", "satellite" and "basic" that doesn't mean anything anymore.. so we'll have to define those keywords to some meaningful non-mapbox styles.

https://github.com/plotly/plotly.js/pull/7015/files#diff-4c999e79e2db6d26fdc437b32284ecb9cf566c275e50c56ad8946b1405e1b27fL176

I see. Now without changing mocks, is there a way to add a function to convert those styles internally at either plotly.js or maplibre level?

[birkskyum]

if some existing user has written "light" (a mapbox style), we can trivially at plotly.js level in the file i linked above have that mean the same as one of the free styles like "carto-positron" or one of the stadia/maptiler/stamen styles (those latter ones would require an api key from users - which isn't set as a property on maplibre-gl, it's just the end of the style urls like the stamen example) - I don't know if that answer the question, of it misunderstood it

[archmoj]

if some existing user has written "light" (a mapbox style), we can at plotly.js level in the file i linked above have that mean the same as i.e. "carto-positron" or similar - I don't know if that answer the question, of it misunderstood it

Great. Let's go with that at plotly.js level possibly here: https://github.com/plotly/plotly.js/blob/7c22d2f988446fabbdbfe3e304789e3045d72ecf/src/plots/mapbox/mapbox.js#L101

Let's declare something like mapboxStyle2maplibreStyle and use it

style: mapboxStyle2maplibreStyle(styleObj.style),

[birkskyum]

I think it might be even easier to to just add the missing keywords 'basic', 'streets', 'outdoors', 'light', 'dark', 'satellite', 'satellite-streets' to this object:

https://github.com/plotly/plotly.js/pull/7015/files#diff-4c999e79e2db6d26fdc437b32284ecb9cf566c275e50c56ad8946b1405e1b27fL29

[archmoj]

I think it might be easier to to just add the missing keywords 'basic', 'streets', 'outdoors', 'light', 'dark', 'satellite', 'satellite-streets' to this object:

https://github.com/plotly/plotly.js/pull/7015/files#diff-4c999e79e2db6d26fdc437b32284ecb9cf566c275e50c56ad8946b1405e1b27fL29

Good call. :100:

[birkskyum]

@archmoj , now there are free styles in place of all the missing keys (arcgis satellite and hybrid, carto / osm for the rest), opening the door for a minor release.

I think it's already possible to add a full style url in the field instead of e.g. "satellite", so that gives full freedom to customize the plots.

[birkskyum]

In either case, I think this work is at a point where it's possible to see a clear path to the finish line. To get much further we need the esbuild in place, so that the CI can pass (and run even faster), and also a decision on the naming of traces, and minor/major release, but that's about it.

The main argument for keeping all the mapbox naming is reducing friction for users to upgrade their code (could be the path for a new minor release). The argument for changing it, at least in the next major, is that some people might look up mapbox api docs or style docs for a scattermapbox, but it's actually maplibre they should look for (this is more likely to not be an issue for now, but for new features like projections the api's will diverge and it's more important to look at the right docs).