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

Breaking changes MapLibre GL JS v4 #3052

Closed
wipfli opened this issue Aug 31, 2023 · 29 comments · Fixed by #3647
Closed

Breaking changes MapLibre GL JS v4 #3052

wipfli opened this issue Aug 31, 2023 · 29 comments · Fixed by #3647

Comments

@wipfli
Copy link
Member

wipfli commented Aug 31, 2023

Let's start tracking breaking changes for MapLibre GL JS v4 in the thread below.

@wipfli
Copy link
Member Author

wipfli commented Aug 31, 2023

I would like to suggest that we start showing a Leaflet-style "MapLibre" in source attribution by default starting with MapLibre GL JS v4.

@acalcutt
Copy link
Contributor

acalcutt commented Aug 31, 2023

@wipfli I was thinking as an alternate to that is we could enable the maplibre logo by default. in the past we had discussed possibly doing that.

Though adding to attribution instead might be less disruptive.

@HarelM
Copy link
Member

HarelM commented Oct 20, 2023

I'm working on the code to reduce as much as possible the callback hell that currently exists.
As part of that, I think there will be a need to change the GeoJSONSource API so that it will return a promise instead of a callback.
Currently all the methods are chainable (returning this) so I can't even ,as an initial step, return a Promise in case a callback is not supplied, which is what I had done with the once.
Generally speaking, it might be a good idea to remove as many of the callbacks as possible from the public API as part of this breaking change version.

@HarelM
Copy link
Member

HarelM commented Nov 2, 2023

The relevant PR of most changes can be found here, it's mostly complete, there are a few things I want to improve in terms of docs, examples, etc, but nothing major ATM:

After digging into this deeply as I have, I'm thinking that there might be a way to improve the addProtocol method.
The current problem with the implementation is that it "ping-pongs" from worker thread to main thread in order to execute the add protocol method.
While this might be OK, I think I have an idea how to extend this capability to allow running things in the worker thread code and avoid this extra "ping-pong".

CC: @AbelVM, @msbarry, @smellyshovel, @bdon.

This is also an opportunity to add some other breaking changes, I'll look into issues and PRs to see if I find a mentioning of things that waited for a breaking changes version and add them here.

@HarelM
Copy link
Member

HarelM commented Nov 7, 2023

To allow reverting back from arrow properties () => ... to "regular" methods, there's a need to return something that you can use to unsubscribe when registering an event listener, I like the rxjs API for that.
This probably needs some more design and thought, but it might be a good candidate for 4.x

@bdon
Copy link
Contributor

bdon commented Nov 13, 2023

Can we take advantage of breaking changes to clean up this? maplibre/maplibre-style-spec#311

@HarelM
Copy link
Member

HarelM commented Nov 13, 2023

I think it's a good idea. Can you push this forward?

@wipfli
Copy link
Member Author

wipfli commented Nov 13, 2023

Just to clarify: a breaking version of MapLibre GL JS means that some public APIs have breaking changes, but the style spec should not have breaking changes. So maplibre/maplibre-style-spec#311 can come also with a minor release of GL JS.

@HarelM
Copy link
Member

HarelM commented Nov 13, 2023

This is a default value change basically, so it's also considered a breaking change, just like the attribution in this thread.

@HarelM
Copy link
Member

HarelM commented Nov 16, 2023

I started with keeping the callback and only adding promises, but weird behavior in the code which I don't think is the right direction.
I plan to change the Source interface to the following as part of this breaking change version:

loadTile(tile: Tile): Promise<void>;
abortTile?(tile: Tile): Promise<void>;
unloadTile?(tile: Tile): Promise<void>;

original:

loadTile(tile: Tile, callback: Callback): void;
abortTile?(tile: Tile, callback: Callback): void;
unloadTile?(tile: Tile, callback: Callback): void;

Meaning, if you created a custom source and implemented the interface, you'll need to return a promise instead of using the callback (which wasn't really checked or used...).

@HarelM
Copy link
Member

HarelM commented Dec 1, 2023

Most of the breaking change related to callback stuff are either merged or there's a PR open.
I'll need to see what other things might be relevant as part of 4.x breaking changes.
@bdon if you would like to push forward the font stuff breaking change, now will be the time, THANKS!

@HarelM
Copy link
Member

HarelM commented Dec 3, 2023

@wipfli @acalcutt what would you prefer?
Adding a default custom attribute with link to maplibre.org:
image
Logo only:
image
Both:
image

The logo looks bigger on desktop:
image

I like the logo only option.

P.S. @birkskyum, can you help with the new logo, seems like we are using the old one still...

@wipfli
Copy link
Member Author

wipfli commented Dec 3, 2023

I personally like the text link in the attribution more. Thanks for looking into this @HarelM!

@HarelM
Copy link
Member

HarelM commented Dec 6, 2023

@acalcutt, any input here?

@msbarry
Copy link
Contributor

msbarry commented Dec 11, 2023

@HarelM are you using a polyfill for AbortController? Looks like AbortController support (96.75%) is lower than async functions (97.08%) which we decided to add a transpile step for, and it's close to webgl2 (95.95%) which we also have a fallback for.

@HarelM
Copy link
Member

HarelM commented Dec 11, 2023

The problem with async was not about polyfills or browser support, but about how angular and other frameworks can't use it in the worker thread due to some webpack or babel limitation.
Webgl2 probably can't be polyfilled, not sure this can be said about AbortController.
In any case, we'll hear back after the release I guess and decide, we can always add a recommendation to add the abortconroller polyfill to the docs if needed.

@msbarry
Copy link
Contributor

msbarry commented Dec 11, 2023

I think recommending users add a polyfill before maplibre loads on main thread is a fine workaround, but that won't work if it's needed in the worker before maplibre code loads. Will your PR #3459 let you import worker scripts before maplibre code loads? If so that might be sufficient?

@HarelM
Copy link
Member

HarelM commented Dec 11, 2023

I believe that when you load the library (without creating a map) it will create the workers and it should be possible using the new API, I added it as a global API for that reason exactly, it's only a theory, I haven't tested it though...

@HarelM
Copy link
Member

HarelM commented Dec 29, 2023

I had an interesting thought just now:
Moving all the UI components out of this package.
This will allow reducing the bundle size and also allow starting a web component library at the same time.
i.e. the new package will hold all the components, it will later on expose them as both classes and as web components and hopefully allow tree shaking of only the components you are actually using.
It might introduce some breaking changes to the options class, maybe, IDK, I still need to think how to build this, but it might be an interesting improvement to v4.
Any thoughts are welcome.
i.e. this library will focus on the map, canvas, gestures, but not the components (zoom, location, etc), although I tend to think that the scale component should be kept here as it feels deeply part of a map I believe.

@wipfli
Copy link
Member Author

wipfli commented Dec 29, 2023

Would that mean that in the future one would need to use a plugin for things like "hash": "map" in the map options? Or is this more about the zoom buttons?

@msbarry
Copy link
Contributor

msbarry commented Dec 29, 2023

Any idea what the size reduction would be? I wouldn't mind this since for example onthegomap uses custom ui controls so users are downloading the built in ones for no reason (also box zoom handler, 3d terrain, heatmap layers, ...). But it's also an ease trade off of use for first time users that they can just depend on one library and get these common widgets with one line of code. Or are you thinking that new users would depend on a default package that includes the base one plus packages for the common widgets?

@HarelM
Copy link
Member

HarelM commented Dec 29, 2023

I'll see how much it reduces, obviously if it's not significant, then the value of this is low.
Yes, mainly the buttons, maybe the marker/popup stuff and basically html that is not mandatory for the map.
This is still a very initial thought, but I want users to simply add another package and receive that with ease.
IDK, I need to properly POC and design this...
The ability to change the UI without changing the core might be an interesting approach I hope.
I also have custom controls in israel hiking map and I don't use most of the built in controls.

@AbelVM
Copy link

AbelVM commented Jan 2, 2024

I love this idea!! The advantages are not only related to tree-shaking (which is a hard requirement in my org, due to LCP times) but also it might expose new public APIs for custom UI components (maybe based on Broadcast Channel API 🤔 )

I had an interesting thought just now: Moving all the UI components out of this package. This will allow reducing the bundle size and also allow starting a web component library at the same time. i.e. the new package will hold all the components, it will later on expose them as both classes and as web components and hopefully allow tree shaking of only the components you are actually using. It might introduce some breaking changes to the options class, maybe, IDK, I still need to think how to build this, but it might be an interesting improvement to v4. Any thoughts are welcome. i.e. this library will focus on the map, canvas, gestures, but not the components (zoom, location, etc), although I tend to think that the scale component should be kept here as it feels deeply part of a map I believe.

@HarelM
Copy link
Member

HarelM commented Jan 11, 2024

In the Monthly meeting it was suggested to create two packages - core and components and a third package to bundle both of them.
I would say that the "bundled" package should have the maplibre-gl name and the two other packages can be names:
@maplibre/components and @maplibre/core.
This is an initial suggestion, this might allow to not create a breaking change.
I don't know, I still need to check the bundle size to see if this worth the effort.

@HarelM HarelM mentioned this issue Jan 21, 2024
8 tasks
@HarelM
Copy link
Member

HarelM commented Jan 23, 2024

I've checked the difference between having the controls in the lib vs not having them in the lib (I simply deleted the relevant code).
The difference is about 6kB after gzip which is around 3% of the total bundle size (213kB vs 207 kB).
If this is something that interest someone or some company to pursue, be my guest, but I don't think we should invest time in this. I think the maintenance overhead is not worth the small change in bundle size, I'm sure there are easier ways to reduce the bundle size if this is something someone is keen on improving.

It's worth mentioning that I have create a POC for web components for maplibre-gl a while back, but I didn't see any traction. So, if someone would like to push this forward and create a "component library" for maplibre-gl-js I think we can use the next major version (5.x) to remove the controls and use this new library, or suggest it at least, but if there's no real push towards it, then it means this is not interesting enough.

I think I made most of the breaking changes I wanted, and we can revisit another major version in 6-12 month.
4.x in now 3 month in pre-release and it's time to release it I believe.
I have a few PR waiting for review, and then I'll release a new version.

@acalcutt @wipfli I haven't received your feedback about the changes related to the logo/attribution, so if no feedback is provided I'll probably keep it the way it is now...?

@wipfli
Copy link
Member Author

wipfli commented Jan 24, 2024

Regarding logo and attribution, I suggest:

  • Do not show the logo by default
  • Show by default "MapLibre" with a link to our website at the beginning of the attribution string

Can you make a pull request for the attribution @HarelM ?

@HarelM
Copy link
Member

HarelM commented Jan 24, 2024

I also need to move the full screen locale strings to the right place.

@HarelM HarelM mentioned this issue Jan 25, 2024
6 tasks
@HarelM
Copy link
Member

HarelM commented Jan 25, 2024

OK, the last two PRs that are linked to this issue is what will be the content of 4.0.
I've just pushed a pre.5 to see what issue it might cause, I'll try to integrate it in ngx-maplibre.
Other than that, I think a new version can be released soon, based on how fast the linked PR will be reviewed :-)

@HarelM
Copy link
Member

HarelM commented Jan 28, 2024

Any last suggestions before we release 4.0? Now is the time :-)

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

Successfully merging a pull request may close this issue.

6 participants