search

maplibre / ngx-maplibre-gl

Angular binding of maplibre-gl
https://maplibre.org/ngx-maplibre-gl/
MIT License
65 stars 25 forks source link

Remove zone.js #165

Open HarelM opened 2 months ago

HarelM commented 2 months ago

I'm not sure if this is possible, and if it's possible, it's probably a huge effort, but it seems that the Angular team is making new features available for zoneless apps. If I understand the direction correctly, they are aiming to remove zonejs as much as possible. I think it might be a good idea to try and move this library to that direction and avoid using zonejs.

Any thoughts on this are welcome. This is more of a hunch at this point in time.

Here's an article that talks about it a bit and it looks like more support for this might come only in version 18: https://netbasal.com/navigating-the-new-era-of-angular-zoneless-change-detection-unveiled-e7404de69b89

So it might be a bit premature ATM. IDK...

marcjulian commented 1 month ago

Here is the official documentation about zoneless requirements.

One way would be to use ChangeDetectionStrategy.OnPush for the components.

If thats not possible:

Components can use the Default strategy as long as they notify Angular when change detection needs to run (calling markForCheck, using signals, AsyncPipe, etc.).

Using signals also means input signals instead of the decorator based version.

HarelM commented 1 month ago

I think that moving to onPush would make this library more performant, but it is also not an easy change, especially since users can add components inside the provided component. Feel free to push this as I'm not sure I fully know how, up until now I was using Angular without looking into performance stuff and adding onPush, since it's a lot easier to reason with, so I'm not experienced enough with onPush strategy...

tbo47 commented 1 month ago

Is it what you had in mind? It's just the beginning https://github.com/maplibre/ngx-maplibre-gl/pull/167/files

HarelM commented 1 month ago

I'm not familiar enough with signals, but sure, if this is how it's done.

marcjulian commented 1 month ago

Is it what you had in mind? It's just the beginning https://github.com/maplibre/ngx-maplibre-gl/pull/167/files

I think that is one way to make it compatible with zoneless. Just curious, why you choose model instead of input signal?

tbo47 commented 1 month ago

yeah I think input would be better.

tbo47 commented 4 weeks ago

I'm in the process of replacing @Input by input https://github.com/maplibre/ngx-maplibre-gl/pull/167

Let me know if it is useful and if it goes in the right direction. @marcjulian @HarelM

HarelM commented 4 weeks ago

I think the direction is good, but I think the main concern regarding zone is how the map events are handled when zonejs is not present. There's a problem with the unit test right now, but we need to fix them in the main branch so that we'll see if the showcase is failing or not.

marcjulian commented 3 weeks ago

By switching to ubuntu as runner the unit test seem to work again (https://github.com/maplibre/ngx-maplibre-gl/pull/170). I can provide a PR which updates to v18 and enables zoneless in the showcase to test the map library.

@tbo47 looks good so far.

marcjulian commented 3 weeks ago

I updated the repo to Angular 18 and enabled experimental zoneless in this PR (https://github.com/maplibre/ngx-maplibre-gl/pull/172). Looks like the map is not rendered in a zoneless application, while already using ChangeDetectionStrategy.OnPush.