Migration to Vue3 and Vuetify3

[sronveaux]

Hi team,

After two months of work on this subject, I think it is time to share a draft of this PR which goal is to migrate Wegue to Vue 3 and Vuetify 3.

Please note this should be considered as a work in progress and is certainly not ready to be accepted as is. I just wanted to share before my summer break so you can all have a look, ask your questions, make some tests, etc...

This was way more difficult than expected at first, migrating Vue and Vuetify could not be done separately as Vuetify is written in a low-level way which makes it totally dependent of the used Vue version. However, I think we're quite close to an update which is not aiming at rewriting the whole framework following new best practices but to transition while changing as few as possible. Improvements can then come afterwards.

Don't hesitate to ask questions linked to implementation choices I've made and why I did them, it's the only way to get all in line and get confidence in the work that was done... and you'll certainly have good ideas that will need changes to be done !

All functionalities of Wegue v2.0.2 are included. The two PRs that came afterwards are not present as keeping in sync after every PR merge could be really tedious. So I decided to sync with new releases. Don't worry, the custom icons functionality was already developed with this migration in mind and what should be changed is already defined and was tested !

This PR currently uses the Vue compat build so that you can make some tests with your current applications and warnings will be emitted if incompatible features are still used inside your codebase. You can also tweak which features are assessed and change the behaviour by changing the properties of compatConfig inside vue.config.js and calls to configureCompat inside main.js and tests/unit/index.js refering to the official features list. Believe me, this was really valuable during the first migration steps and at the end to ensure no legacy behaviours were still present !
The only drawback is that we can't compare performance and build size with current Vue 2 implementation for now as this compat build induces some overhead. But it can easily be replaced by the pure Vue 3 build by removing the resolve alias inside vue.config.js and commenting the calls to configureCompat...

Concerning functionalities, there are certainly a few glitches here and there (don't hesitate to report !), but here's the list of known issues that are still present for now :

1. AttributeTable is a bit slow as it seems to freeze instead of displaying the "loader" while reading layers data. I didn't have time to investigate on this one for now...
2. Choosing the base map with the keyboard doesn't work anymore. I think this could be a Vuetify bug as we can see tabindex being placed on the wrong elements... activating / deactivating modules from the top/right menu works with the keyboard though which is an automatic nice addition !
3. Unfortunately, layer properties are still non-reactive and worse, the layers list in itself is non-reactive ! I thought I've found a "simple" trick that made everything work again (I'll explain details in OL layer properties are no longer reactive #365) ! But a little later, I discovered it was not a good solution... it broke OpenLayers internals and selection of features was not working anymore... So I reverted and implemented something so this PR works as current version of Wegue is (layers list is 'reactive' but layers properties are still not) however, I consider this a potential regression... this is to be discussed for how to go further. Perhaps we can reuse what @fschmenger proposed in Fix layer property reactivity #368 or I thought about making some tests with a shallowReactive or a customRef... All this is present in the latest commit so this one can be reverted to see the buggy trick I used at first... perhaps this could be easily fixed too I don't know... it will take some time to investigate in all cases...
4. I just discovered a bug which is present in current version of Wegue (so this is not introduced by this PR !) and seems to be linked to infoClick on multiple visible features/layers #367... if you open the InfoClick, select some earthquakes, consult other items than the first then try to use the Measure tool for example, you'll get an exception about undefined features. I report it here for completeness but this is not directly linked to this PR...

There are some visual differences due to changes in Vuetify implementations. Some I have tackled as I thought the new visual was really bad (font applied to names of the layers in the base layer switcher for example) but I let the rest with the new styling so we can discuss what to change or not. For example, you can see how sliders are now rendered by default.

Concerning unit tests, there are three very important things to mention here :

1. It seems there is a bug inside vue-test-utils which makes the test suite unrunnable as is. I opened an issue there with suggestions about what could be changed to correct the problem. Discussions are ongoing and I hope I'll get the approval to open a PR there to correct the problem. In the mean time, you can manually invert two lines in your local installation inside node_modules to correct the problem. Just have a look at the VTU issue
2. There is a bug in Vue-I18n where changing of locale inside a unit test breaks translations for the rest of the test suite. To avoid creating a new Vue-I18n instance in each and every test, a global instance was created in tests entry point. To bypass this problem, the only (almost) clean way I've found for now is to create a new Vue-I18n instance in the tests that change the locale. This works, except those tests emit warnings telling a plugin which was already defined is re-defined...
3. Some new unit tests should be written for changed and new functionalities (composables, WguEventBus...) but this will be done in the end when and if implementation choices are accepted by all of us...

O.K., I think this text is certainly long enough but there you have all the info concerning current state of this PR. Don't hesitate to ask all the questions you have so we can go further (this will certainly take a long time) but don't expect answers before the end of August...

All the best,
Sébastien

[sronveaux]

Hi team,

Following the release of Wegue V2.1.0, I updated this PR to include the changes and ported what needed to be to Vue3 and/or Vuetify3.
I also updated Vue, Vuetify, Babel, Karma, Chai, Sinon, ESLint, Vue-I18n, core-js and their dependencies to the latest compatible version. The most important updates are Vue going from V2.4.x to V2.5.x and Vuetify going from V2.6.x to V2.7.x

Here's a quick recap of the issues listed when this PR was created:

• Cannnot run unit tests without manually patching Vue-test-utils (Solved by creating a PR in vue-test-utils)
• Problem with unit tests which need to change the locale in Vue-I18n (Solved by using a global mock as Vue-I18n team is not reacting to the idea of solving the source problem)
• Bug in InfoClick (Solved in V2.1.0)
• AttributeTable is sometimes slow when opening the combobox or choosing a layer (Had no time to look why yet... to be solved separately later ? Edited: Solved)
• Base maps cannot be switched using the keyboard anymore (To be solved separately later ?)
• Layer properties are not reactive anymore (It is already the case in V2.x ... to be solved separately later ?)
• Some unit tests on new features should be added (To be added separately ?)

Following the opening of discussion #423 I think it would be useful to define what should be addressed in this PR and what could be done separately afterwards.

One last point we should all agree is whether the compat build of Vue should be removed from this PR or at a later stage. It would be cleaner to remove it now but could also be useful for you to verify compatibility with your current apps...

Don't hesitate to give your opinions and feelings here and ask all the questions you could have concerning implementation choices that were made in this one so we can all keep in-line!

Cheers,
Sébastien

[chrismayer]

AttributeTable is sometimes slow when opening the combobox or choosing a layer

➡️ IMHO this could be solved separately later

Base maps cannot be switched using the keyboard anymore

➡️ IMHO this could be solved separately later, after merging this PR but should be a must for v3

Layer properties are not reactive anymore (It is already the case in V2.x)

➡️ IMHO this could be solved separately later

Some unit tests on new features should be added

➡️ IMHO this could be solved separately later

whether the compat build of Vue should be removed from this PR or at a later stage

➡️ no strong opinion on that

[sronveaux]

Hi team,

As I couldn't go without at least looking what was going wrong with the AttributeTable, I've looked and found the problem. It is now corrected !

So only two known problems left... however, following latest discussions, I gave a try to replace the Mapable mixin by a composable as this is a major change which should be done now or would have to wait for a V4 in the future...

I'll post the update later today and comment it so you can all have a look at what has changed, what it would imply to migrate to it in custom apps and whether we go in that direction or revert to the previous commit...

[sronveaux]

As promised, here's a proposal of a map composable written to replace the mapable mixin and to make the layers list reactive.

This was written with the same logic as the rest of this PR to minimize what was modified.

As you'll see, migrating to this implementation would imply to import the composable in place of the mixin, remove the mixin declaration and call the composable inside the setup lifecycle hook from which we can get back the map and/or the reactive layers array.

Unfortunately, it is not possible to trigger a callback from the composable as it was done before with the mixin where a component could define a onMapBound and/or a onMapUnbound to be called when the Map component would emit some events on the WguEventBus. It could be possible if the callback was passed as argument in the call to useMap, however, this would mean the callbacks should be defined in the setup hook as all the data they should work on... in other words, this could be possible if all the components were rewritten with the Composition API instead of the Options API. As this PR tries to minimize the changes, this is not an option here. Perhaps this will be one day in a future version...

If you look at the changes made in this PR, you'll see there was different ways for components to get the map instance. Some were using the mixin, some were registering a callback on the on-map-bound event, etc... what I did here is uniformise the way the map is received, all components use the composable. When a callback was required, what I did is create an immediate watch on the map instance. Inside it, we can easily call the onMapBound and/or onMapUnbound if needed.
With the watch being immediate, we can test the old and new values and address the different scenarii that existed in the current mixin implementation where we had to look whether the map already existed or not and sometimes registering a callback, sometimes not. It is not as neat as I would like it to be but is in a sense way nicer and cleaner than what was done lately to bring back reactivity to the layers array.

Another nice thing is the fact that the Map component has no need to use the getCurrentInstance private API anymore. Even though it is often used even by big projects such as Vuetify and will certainly not be removed in the future, relying on undocumented features is never a good thing when you can do things differently.

As you'll see, I tried to modify as few as possible, that's why you'll see some "strange" things like in the MeasureWin where the unbound variable is managed as it was before in the mixin even though I don't really think this is still useful... potential cleaning could be done in some components but I think this should be done later if we want to and if the composable way is the one followed...

As expected, those changes break a lot of unit tests. Locally, I have 89 test failing for now... however, before adressing them, I wanted to get your impression about whether we keep this composable or whether you'd prefer to revert the latest commit... if you think we can continue on what was done here, I'll fix those tests shortly, clean the code and change the comments so we can go further on the roadmap to Wegue V3.

Edit: I just refactored the unit tests to use the new map composable. Everything is green again ;-) so if we decide to keep the composable, only the code cleanup and comments adaptation are needed...

Just tell me what you think about this and I'll adapt accordingly !

All the best,
Sébastien

[chrismayer]

This is really huge @sronveaux! Big thanks for this 👍

I tested this by running the example applications. Looks really good. Everything seems to work so far. Really good job 💪

Having a review line-by-line seems nearly impossible. But I had a scroll through and it looks good and the changes are understandably so far.

I vote to go on with the composable approach for the OL map. Since it is already implemented and works fine we should use it. And using watch instead of the onMapBound callback looks good and feels a bit more "vue-like". I discovered that you only commented out the Map-mixin code. Would you please clean the code and remove it at all? After that I am in favor to merge this. I'll create a legacy v2 branch in the meantime, but more to this in the v3 roadmap.

[sronveaux]

Hi @chrismayer and thanks for your nice comment !

Great, I'll continue in this direction as soon as I've got time then !

It's perfectly true that Mapable mixin code was just commented as I thought it would give a better idea of what would need to be changed if the useMap composable was kept in replacement of the mixin, giving you all the needed information to take a decision. I'll clean all this (and potentially other comments that I'd have forgotten in this lengthy process) and let you know as soon as it will be.

[sronveaux]

Hi @chrismayer,

Here we are, ready for a Christmas PR !

As agreed, I cleaned the source code and added some comments for the new implementation where needed.
I also a corrected a missed bug and changed the vue configuration file so that sourcemaps are generated properly once again.

I also took the opportunity to correct some typos and uniformise unit tests structure where possible.

Eventually, I also updated almost all the dependencies before publishing this PR. As it will induce lots of breaking changes, it is the right moment to do it I think...
Potential missing potential updates are:

• canvas-record : I must admit I didn't look at it at all...
• ol and proj4 : In general, you are the one updating OpenLayers, but never proj4... I don't know if there is a good reason or not but thought updating them could be done separately after discussing with you about this...
• @mdi/font and material-icons : They are already in the roadmap and should be updated separately...
• eslint : Should stay in v8.x branch until Transition to new ESLint configuration #379 and Use new code formatting approach since ESLint deprecated all formatting rules #380 are addressed... this could be possible soon I think... if you agree on my latest proposal there... until then, it is pinned to the latest v8.x version...
• gh-pages : Can be addressed separately if needed...
• sass-loader : Updated to latest "secure" version. No need to try doing more as it will be removed when transiting to Vite...
• chai and sinon-chai : Pinned to latest version compatible with Karma. Newer versions are incompatible due to dropped CommonJS compatibility which is required by Karma...

Considering the roadmap, I think the two items to be done before merging are now fully addressed. I also think the "Update documentation" in must-have list is done too, even though I'll keep an eye when writing the migration guide to ensure it is the case...

All this would deserve to be tested once again to be sure it works as expected, but I now consider it as a "potentially good" PR and will thus remove the draft status from it.

I also take the opportunity to wish a merry Christmas to the whole team and hope the followings (surely in 2025) will be exciting!

Cheers,
Sébastien