Popup click changing routes results in NG0953

[marcjulian]

I have an application where a Popup display data and uses RouterLink to change the route on click.
When the route changes the popup is closed which emits the popupClose output. However, the component is already destroyed. This breaks the navigation resulting in a white screen.

I will try to add an example for this

[magnolo]

I stumbled across the same error when using @for around <mgl-layer /> and tried to get track by the layer ID. Our system then only updates the paint properties of the layer and didnt update because the paint object reference didnt change. I tried then to update the id of the layer when a paint property changes (not a good idea i know =). So a new layer gets created and the other the should be removed, but the removed layer (maybe) tries still to emit something.... sry for the confusing description.

Error: maplibre-ngx-maplibre-gl.mjs:245:47

Tried to create an example of all that: MapTest Stackblitz

EDIT:
I played around a bit more and it seems also to happen, when i remove a layer and try to add a layer with an ID that existed before

[HarelM]

Looking at the MapService addLayer code, it looks like when removing a layer there might be a need to unregister all the registrations that were made when the latter was added.
It's worth a shot if you want to try it out.

[magnolo]

hey there. Thanks for the fast response. I just digged into the code a bit and if i am not totally mistaken i need the listener function itself (reference) which gets registered on the mapInstance.on call, to unregister it later on to call mapInstance.off within removeLayer. So we have to store it somewhere. This should then also apply to all other mapInstance.on calls that are not global map related ?

i am not so familiar with this library code. so i will just get into it more deeper and hopefully can come up with a solution or maybe a good suggestion.

Wish i could remove all events from the mapInstance with just the eventTypeName and the layer ID, but as far is i know that is not possible.

[HarelM]

I tend to think you got it correctly.
I would try and stove this only for a specific event and see if it actually solved the issue before doing it for all events.

[magnolo]

So i played around and testet this with only the layerMouseMove event and stored the function within a mapService variable. When i call the off function for the mapInstance within removeLayer with the function reference, no error gets thrown.

Now, i think, it would just be a matter of where to store the listener functions

[magnolo]

i got another confession to make. The base problem i stumbled across all this, is because i edit the layers paint and layout properties. Now i realized, that when i update layout or paint, the properties that are removed from previousValue to currentValue are ignored, because setAllLayerPaintProperty and setAllLayerLayoutProperty are only handling the current available keys.

i am on it =P

[HarelM]

I have the same issue now with one of my popups in my latest version.
@magnolo @marcjulian did you find a solution to this?

[HarelM]

I see that ngOnDestroy was removed from the popup component class as part of

• feat: Migrate decorators to signal queries #178

I'll create a PR to bring it back unless @n-elhk can explain why it was removed and how to solve the current issue at hand.

  ngOnDestroy() {
    if (this.popupInstance) {
      if (this.lngLat || this.feature) {
        this.mapService.removePopupFromMap(this.popupInstance);
      } else if (this.marker && this.marker.markerInstance) {
        this.mapService.removePopupFromMarker(this.marker.markerInstance);
      }
    }
    this.popupInstance = undefined;
  }

[HarelM]

My mistake, this method was just renamed and is used from destroyRef now...

[HarelM]

I think I was able to create a minimal reproduction of my scenario:
The problem happens when there's a popup attached to a marker, and both are destoried.
The marker gets destroyed first so when trying to remove the popup, the marker is already null, I think.
I'll dig deeper and see what I find...

[HarelM]

Interestingly enough, calling the same method from ngOnDestory is working as it did before, because somehow the marker is removed only after ngDestory is being called and so the removal of the popup succeeds.
I'm not sure what to do with this information.
We can revert to using ngOnDestroy, but that's not a great solution.
We can open a bug for angular, which I doubt will be solved in a timely manner.
I can't think of a better solution.
I'll sleep on it...

[HarelM]

I've checked the situation in a previous version (17.4.3).
The behavior is basically the same, this is probably a new warning angular added in version 18 to let the developer know that this is happening.
Still not sure how to properly solve this. Any input would be appreciated.

[magnolo]

I havent figured out yet, about the removing and readding layers with the same id problem (have not got time enought yet). But i just tried reset layout and paint properties if some of them got removed.

magnolo@c36e9c2

will help to investigate more here. but i sadly dont know when to take the time.

[n-elhk]

Interestingly enough, calling the same method from ngOnDestory is working as it did before, because somehow the marker is removed only after ngDestory is being called and so the removal of the popup succeeds. I'm not sure what to do with this information. We can revert to using ngOnDestroy, but that's not a great solution. We can open a bug for angular, which I doubt will be solved in a timely manner. I can't think of a better solution. I'll sleep on it...

I investigated and found something surprising: destroyRef.ondestroy and ngOndestroy are not triggered at the same time.

It seems to me that the ngOndestroy life cycle is called when all its children have been destroyed.

whereas ondestroy is called as soon as the component is destroyed, regardless of the state of its children.

[HarelM]

I've observed this too, but I don't think this is the problem in this case.
<mgl-marker> and <mgl-popup> are siblings in my case so there's no need to wait for a destroy of an inner child.
I think this is a new warning by Angular, which we need to decide how to solve.
The fact is that a popup is being destroyed and the close event is never unregistered...
I think we have this problem for other registrations as well.
A possible way to move this forward in a more infrastructure solution would be to allow receiving a subscription kind of return value from "on method" in maplibre-gl or to use rxjs' fromEvent more here... IDK, these are only some initial thoughts...

[HarelM]

@n-elhk do you remember why you changed the value of the second parameter below from false to true?

ngx-maplibre-gl/projects/ngx-maplibre-gl/src/lib/popup/popup.component.ts

Line 207 in 04b99bb

this.mapService.removePopupFromMap(this.popupInstance, true);

Was it the same warning in the console? I'm trying to figure out if and when a close event should be emitted if the popup is destroyed vs simply closed...