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

OL layer properties are no longer reactive #365

Open
fschmenger opened this issue Feb 22, 2024 · 8 comments
Open

OL layer properties are no longer reactive #365

fschmenger opened this issue Feb 22, 2024 · 8 comments

Comments

@fschmenger
Copy link
Collaborator

It seems that lately vue-bindings to layer.getProperties() or via the layer.get('someProperty') will not get updated. Particularly puzzling is, that it seems to work for all layers added in the 1st generation, i.e. those configured statically in app.conf but not for those added dynamically in a later cycle.

To see an example: Create a drag drop layer (e.g. download GeoJSON from https://openlayers.org/en/latest/examples/drag-and-drop.html) and open the layer list module. Then switch the active language. The drag drop layer is localized and supposed to switch its language, however only the "static" wegue layers will receive the update.

The related code in LayerListItem looks like

<v-list-item-title>
        {{ layer.get('name') }}
</v-list-item-title>

Neither does it work via getProperties() if you alter the code above to

<v-list-item-title>
        {{ layer.getProperties().name }}
 </v-list-item-title>

or by using explicit computed properties, etc.

Note that some other components, i.e. the AttributeTable will still update the layer name. This is just a happy circumstance because they use other localized attributes in the same block, causing the whole thing to re-render.

Trying to track it down I rolled Wegue all the way back to v1.2.1 to finally see it working. However, if I delete the package-lock.json and do a full reinstall of dependencies, then the functionality is broken even back then.

A workaround could be to register for layer propertychange events and then manage our own binding targets, e.g.

layer.on('propertychange', (evt) => {
   if (evt.key === 'name') {
       this.layerName = layer.get('name')
  }
});

We used to have something like this in a very old version of Wegue but it would be cumbersome to reintroduce it and hunt down all code blocks where a binding to layer properties occurs.

Any ideas on the underlying cause and what to do about it?

@sronveaux
Copy link
Collaborator

Hi @fschmenger,

I tried to have a look at this one and arrive at the same results as you for now...

To give some more details of what I saw, if you install dependencies on Wegue v1.2.1 then remove package-lock.json and try to recreate it, the problem doesn't happen neither. You have to recreate the package-lock.json with an empty node_modules directory for the problem to occur. In this situation, the number of changed dependencies is so big that tracking what could induce this could potentially take ages...

Another funny thing is that when you reproduce the bug with the drag and drop as you explained, the name is correctly regenerated once you hide or show the layer by clicking on the chekbox next to the name... then if you switch languages again, you once again have to hide or show it for the name to be regenerated...

I tried rewriting the LayerListItem component to generate the name based on a deep watch on the passed layer property. We shouldn't do it, but it was for testing purpose only. Here clearly, the watcher is not called when languages are switched for this particular layer where it is for the other ones...

Last thing I saw is that if I force the measure layer to appear in the layers list (OlMeasureController.js line 47), it has the same problem, the name doesn't change when languages are switched. This one is created afterwards in a onMapBound hook but always at startup, which is not the case with the drag and drop layer.

I'm not sure this comment will help and I must admit I have no real clue on what happens here for now... I'll keep it in my head though and will come back to it if something comes up in my mind...

@fschmenger
Copy link
Collaborator Author

Hi @sronveaux,
Thanks for the investigation. I followed a similar path when looking at it...
First of all I thought the LayerCollection was to blame, because we're also relying on Collection.getArray() to be reactive. This however works, as the list itself is updating. Then I tried creating a layer, adding it to the map and binding to it directly (without any indirections). So the problem is definitely around the layer object itself or the layer propeties.

A good starting point could be to look in the OpenLayers property management which is handled inside BaseObject. My first guess was, that under the given circumstaces a new instance of the property object managed inside this.values_ is created via Object.assign. I'm not sure if this could cause reactivity to fail, but probably we`re running into something mentioned in Change Detection Caveats. Just a rough guess...

Maybe it`s also interesting to use one of the debugging options in https://vuejs.org/guide/extras/reactivity-in-depth.html?!
As far as I understand there are some changes in this respect in Vue3. So if you looking into migration things someday, you could check if the problem goes away magically?!

@fschmenger
Copy link
Collaborator Author

I finally understood the source of the problem. From the aforementionened Change-Detection-Caveats:

Vue cannot detect property addition or deletion. Since Vue performs
the getter/setter conversion process during instance initialization, a
property must be present in the data object in order for Vue to convert it and make it reactive. For example:


var vm = new Vue({ data: { a: 1 }})
// vm.a is now reactive
vm.b = 2
// vm.b is NOT reactive

The cause is obviously that we're adding the name and attribution properties after layer creation in Map::updateLocalizedLayerProps. These are reactive for the 1st generation of layers, since map initialization is early and all constructed vue bindings to layers occur after that. However it won't work for dynamically added layers, as the initial established binding will see a layer property object without e.g. the name property.

To check the difference, e.g. add a name:'' property to the drag drop layer right on creation in Map::createDragDropLayer.

This is overall quite unfortunate. I can only assume it worked in an older version, because the layer:add event - registered in Map::updateLocalizedLayerProps - fired before any bindings were initialized.

Now we have to think about a solution :/

@sronveaux
Copy link
Collaborator

Hi @fschmenger,

Good job, that's exactly it !

Well, at least we have a quick and dirty hack to make it work as expected if this is of importance inside a given project... how to do it in a clean way is something else... as you said, let's think about a solution... :/

@fschmenger
Copy link
Collaborator Author

Hi @sronveaux,
I was really scratching my head about a viable solution.

I'm currently working on a draft based on proxy objects which would wrap up OL layers (and layer collections). The layer proxy will just override the get and getProperties behaviour and forward everything else to the underlying OL layer, therefore behaving mostly transparently. Then we could simply exchange layers via this wrapper in modules where reactivity is required.
This is the best I could come up with.

Even though I probably won't have time to finish it by now, I will post a first draft later on, so you can have a look at it.

@sronveaux
Copy link
Collaborator

Hi team,

As I promised in #411 here are some things I saw and tried on this subject while upgrading to Vue 3. @fschmenger you asked if I could check how this behaved in this case, that's why I didn't react at first as I knew this was coming... but we all know, sometimes plans drift as we have other mandatory things to do...

First of all, the situation in Vue 3 is in fact more problematic than in Vue 2 because here, even the layers list is non-reactive !
This can be easily explained due to differences in reactivity implementation in both versions. As you have certainly read, Vue 2 added properties on reactive objects (using Object.defineProperty) where Vue 3 'replaces' reactive objects by Proxies.
This opens new possibilities compared to Vue 2 but has unfortunately some drawbacks in cases like ours. Before, we can imagine the layer collection had the properties needed for reactivity injected by a component where in Vue 3, if changes are not made through a Proxy, reactivity will not happen...

I thought a nice trick was to create a reactive layer collection inside the Map component and to inject it on our map. That way, all components would get this Proxy when calling map.getLayers() and reactivity would work ! At first, this worked and even layer properties were, once again, reactive !

Unfortunately, there is another drawback that is well explained in Vue documentation. Comparing a reactive object to the original one is not (easily) possible anymore as you compare the Proxy to the original object. This has some consequences when you compare layers for example and have to compare the source layer with toRaw(layer) to ensure you get the proxied object and not the proxy itself.
I could easily make changes so layer comparisons were fully functional as well as hover functionality which was also not working anymore for the same reason.
However, it seems some OpenLayers internals are broken when doing this. With this implementation, it is impossible to select features on vector layers. I suppose there are some tests inside OpenLayers which compare Proxies with objects and which break...

Those results are relatively new in my mind so I don't know how to go further from here... one way could be to fully test what you did in #368 to be sure it doesn't break some other OpenLayers functionalities and implement it everywhere reactivity in layers is needed, another possibility would perhaps be to use the new tools available in Vue3 like shallowReactive or creating a customRef which could then be triggered on specific operations... this would also be touchy I think...

@fschmenger
Copy link
Collaborator Author

Hi @sronveaux,
thanks for looking into this!
It makes alot of sense to hold back #368 and test it in conjunction with the Vue3 migration.
As you pointed out, Vue3 might offer some easier solutions, so we don`t have to maintain our own proxy objects.
It would be definitely interesting to have a deeper investigation of those new concepts.

I'll be off for vacation for the next 3 weeks, so don`t expect any feedback in the meantime :)
Cheers
Felix

@sronveaux
Copy link
Collaborator

Hi @fschmenger and thanks for the fast reply !

Same for me, I wanted to share all my latest work before my summer break... so don't worry, I won't delve more into this for the next weeks neither ;-)

Cheers,
Sébastien

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants