maplibre / ngx-maplibre-gl

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

feat: Migrate decorators to signal queries #178

Open n-elhk opened 2 weeks ago

n-elhk commented 2 weeks ago

Migration of the following decorators:

Migrate to new methode for dependency injection :

constructor(myService:MyService){} => myService = inject(MyService);

HarelM commented 2 weeks ago

Is this PR ready for review or still in progress? Can you change it to draft and use "request for review" when completed if this is still in progress?

n-elhk commented 2 weeks ago

Yes this PR ready for ready for review, another PR to is needed to change the @Input decorators as this requires more time

HarelM commented 2 weeks ago

Would it be better to wait for the input change as well before merging this? I would prefer to have everything in the same PR so that the code won't be partially "new style" and partially "old style". Does it make sense?

n-elhk commented 2 weeks ago

In fact, it doesn't change much, but it's true that it's better to have everything in the same "style".

HarelM commented 2 weeks ago

Thanks for the input, I'll keep this PR open and wait for the input changes.

n-elhk commented 1 week ago

I made the changes , PR is ready for review

HarelM commented 1 week ago

I haven't finished the review yet, sorry, I will continue tomorrow...

HarelM commented 1 week ago

I've finished my review. Thanks for all the work around this PR!! A few general comments: I see places that use destroyRef.onDestory and other places which we use ngOnDestory, I think we should use the same for everything if possible.

There are places that uses signal for what were private properties, can you elaborate on my use them in this case?

This is a very large change, I've changed in my app a places with takeUntilDestoryed vs subscription list and got a nasty error. How do you know everything is working as expected? I've not sure the tests coverage here is really covering all the components unfortunately...

n-elhk commented 1 week ago

I saw that the unit and integration tests did not cover the entire app to check if I didn't break anything. I compared it with what is in production by replaying the showcase.

HarelM commented 1 week ago

There are a few last comments. Sorry for the long review...

HarelM commented 1 week ago

Can we just composition instead of inheritance for the sources?

n-elhk commented 1 week ago

Done

HarelM commented 1 week ago

Ok, I'll do another final review, if I won't see anything critical I'll merge this. Thanks for all the work on this @n-elhk !!

HarelM commented 4 days ago

Can you explain the change from a local property in a class to a signal type? What's the pros and cons of this approach?