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,

Just some news concerning unit tests in this PR.

1. The bug with vue-test-utils is now corrected. I created a PR which is now merged. No new release has been made since though, so currently, vue-test-utils is installed directly from GitHub, taking the commit where the PR was merged. Obviously, I will use the latest release on NPMJS as soon as it's out...
2. The bug with Vue-I18n was found, I have a working workaround for it but have the feeling it will be way harder to get it included directly inside the official package. So the latest commit implements another workaround which also works but isn't really nice from my point of view. In summary, Vue-I18n isn't registered as a global plugin anymore, just $t is added as a global mock returning the required translation key. This is a common way to cope with Vue-I18n inside unit tests... The drawback is that you must overload the mock to effectively call Vue-I18n $t implementation in tests where translations are really needed (this concerns tests of the MeasureResults component). This is not really nice... however it fits with the generally accepted way of doing it. We often see considered as a bad practice to register a Vue-I18n plugin as it is considered you start testing it instead of your own logic... just tell me how you feel about it and which way to continue on... another possibility which could be better would be to spy on the $t global mock instead of overloading it and ensure it was called with the correct parameters instead of testing the exact string it returns as the tests are doing now...

Just tell me what you think about this last point so I know if you'd prefer me to refactor the MeasureResult tests or not. I think this would be less clear (nicer to test a result is equal to 1 km² as now) but more in-line with best practices (see whether $t was called with wgu-measuretool.areaSquareKm and [1] as parameters)...

[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