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

Remove zone.js #165

Open
HarelM opened this issue Apr 21, 2024 · 11 comments
Open

Remove zone.js #165

HarelM opened this issue Apr 21, 2024 · 11 comments

Comments

@HarelM
Copy link
Collaborator

HarelM commented Apr 21, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator Author

HarelM commented May 23, 2024

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
Copy link
Contributor

tbo47 commented May 23, 2024

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

@HarelM
Copy link
Collaborator Author

HarelM commented May 23, 2024

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

@marcjulian
Copy link
Collaborator

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
Copy link
Contributor

tbo47 commented May 24, 2024

yeah I think input would be better.

@tbo47
Copy link
Contributor

tbo47 commented Jun 5, 2024

I'm in the process of replacing @Input by input #167

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

@HarelM
Copy link
Collaborator Author

HarelM commented Jun 5, 2024

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
Copy link
Collaborator

By switching to ubuntu as runner the unit test seem to work again (#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
Copy link
Collaborator

I updated the repo to Angular 18 and enabled experimental zoneless in this PR (#172). Looks like the map is not rendered in a zoneless application, while already using ChangeDetectionStrategy.OnPush.

@marcjulian
Copy link
Collaborator

marcjulian commented Aug 14, 2024

Changes required:

// app.config.ts
export const appConfig: ApplicationConfig = {
  providers: [
-  provideZoneChangeDetection({ eventCoalescing: true }),
+  provideExperimentalZonelessChangeDetection(),
    provideRouter(routes),
    provideHttpClient(withFetch()),
    provideAnimations(),
    provideClientHydration(),
  ],
};

Remove zone.js from polyfills in angular.json.

Remove NgZone.onMicrotaskEmpty, NgZone.onUnstable, NgZone.isStable, or NgZone.onStable (https://angular.dev/guide/experimental/zoneless#remove-ngzoneonmicrotaskempty-ngzoneonunstable-ngzoneisstable-or-ngzoneonstable) (#183)

marcjulian added a commit that referenced this issue Aug 22, 2024
… zoom, closes #188

* need to figure out, how to replace for #165
marcjulian added a commit that referenced this issue Aug 26, 2024
* revert removing zone.onMicrotaskEmpty to cleanup cluster correctly on zoom, closes #188

* need to figure out, how to replace for #165

* unsubscribe from zone.onMicrotaskEmpty

* zone.onMicrotaskEmpty subscription already unsubscribed because of  subscription list

* remove unused import

* replace zone.onMicrotaskEmpty with afterRender to call applyChanges

* rename applyChanges to clearMapElements
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

3 participants