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

feat: enable zoneless app #191

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: enable zoneless app #191

wants to merge 2 commits into from

Conversation

n-elhk
Copy link
Contributor

@n-elhk n-elhk commented Aug 27, 2024

Update to Angular 18.2.5

Add ChangeDetection.OnPush to all demo components

Replace provideZoneChangeDetection by provideExperimentalZonelessChangeDetection()

Use signal to update the template

@n-elhk n-elhk requested review from marcjulian and HarelM August 27, 2024 10:40
@n-elhk n-elhk force-pushed the feature/remove-zone branch 2 times, most recently from 751cf66 to 51e2943 Compare August 27, 2024 11:15
@HarelM
Copy link
Collaborator

HarelM commented Aug 27, 2024

Can you please split this PR into different PRs, each with a smaller scope?
If there are lint changes for example that are done automatically then reviewing is not interesting, so if not every bullet above as a separate PR, then at least the automatic stuff.
THANKS!

@n-elhk
Copy link
Contributor Author

n-elhk commented Aug 27, 2024

so if not every bullet above as a separate PR, then at least the automatic stuff.

I can split it into 3 Prs this way:

1st: update node version

2nd: update angular version

3rd: the rest

@HarelM
Copy link
Collaborator

HarelM commented Aug 27, 2024

No need to update node as node 22 is not the current latest stable version.
Lint (change quote between single and double) should be a different PR.
I just recently updated the Angular version see

So I would focus on a PR that only enables zoneless app.

</mgl-marker>
}
}`,
@for (feature of clusterPoints(); track $index) { @if
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this your IDE setting that is setting the "for" and the "if" in the same line?
I don't think it adds readability...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also have the same formatting issue with VSCode in this project. It wont format the new Angular Control Flow syntax correctly and also changes single quotes to double for me as well. How to you format the code?

I usually use prettier for formatting

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually prefer lint, although usually not as powerful, it gets the job done.

@n-elhk n-elhk force-pushed the feature/remove-zone branch 6 times, most recently from dc3247e to ffb8fc3 Compare September 21, 2024 08:35
@n-elhk
Copy link
Contributor Author

n-elhk commented Sep 23, 2024

Hello @HarelM @marcjulian

I have a warning in the path 'demo/add-image-missing-generated'.

This warning is triggered when I replace 'provideZoneChangeDetection' with 'provideExperimentalZonelessChangeDetection'.

The 'maplibre-gl' package raises this warning:

image

I investigated and here's what I found:
when initializing the mgl-layer component with the init() method, the addLayer function is used and this causes this warning.

Do you have any idea how to solve this problem ?

@HarelM
Copy link
Collaborator

HarelM commented Sep 23, 2024

Are you sure the styleimagemissing event is triggered and the img-layer is created to add an image to the map and thus avoid this warning?
It looks like the image is not added to the map, or maybe added asynchronously thus maplibre-gl is complaining that the image is missing...

@n-elhk
Copy link
Contributor Author

n-elhk commented Sep 23, 2024

Are you sure the styleimagemissing event is triggered and the img-layer is created to add an image to the map and thus avoid this warning? It looks like the image is not added to the map, or maybe added asynchronously thus maplibre-gl is complaining that the image is missing...

Yes, the styleimagemissing event is triggered and the images are displayed correctly.

@HarelM
Copy link
Collaborator

HarelM commented Sep 25, 2024

It might be a timing issue then, not sure...
The following is the same example, but without the angular wrapper - this seems to work without console warning or errors:
https://maplibre.org/maplibre-gl-js/docs/examples/add-image-missing-generated/

@n-elhk n-elhk force-pushed the feature/remove-zone branch 5 times, most recently from aa062aa to f6fe638 Compare September 28, 2024 08:23
@n-elhk n-elhk force-pushed the feature/remove-zone branch from f6fe638 to 52aebbd Compare September 28, 2024 08:33
@HarelM
Copy link
Collaborator

HarelM commented Oct 6, 2024

@n-elhk can you please take a look at the issues that are open, I think the migration to signals introduced some issues especially with removed destroy methods.
It is best to make sure that everything is working as it did before, before rushing to integrate new features.

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 this pull request may close these issues.

3 participants