Update to last Vue CLI v5 template

[sronveaux]

Hi team,

As I had a little time, I took it to create this last PR in the Vue-CLI upgrade series.

This PR is again way smaller than #312 and should be way shorter to review too... the most difficult part was to make karma-webpack work.

This one is based on the latest Vue CLI V5 template (5.0.8) which was published on the 7th of July 2022.

To stay in tune with #297 (comment), minimal versions of node and npm have been set according to the official Vue migration guide where it is stated that support for Node.js versions 8-11 and 13 were dropped. So once again I took

"engines": {
    "node": ">= 14.15.0",
    "npm": ">= 6.14.8"
}

to use the first LTS version among the V14 ones.

Cheers
Sébastien

[fschmenger]

Hi @sronveaux.
Thanks a lot for providing this so quickly. I`m currently a bit limited on time, but hopefully can have look into it in the upcoming weeks.

[sronveaux]

Hi @fschmenger,

Thanks for your reply, take your time, there's no hurry. I had a little time and everything was ready (just a few commits to cherry-pick) so it's here.

Hope you'll like it !

Cheers,

[fschmenger]

Hi @sronveaux.
I finally went through the changes step by step and tested everything in a larger custom application First of all thanks alot for the fabolous work! I can see that the series of migrating Wegue to a stable build environment is finally coming to an end. Very nice also, that the previously introduced workaround related to Open SSL MD4 support is no longer necessary. So this runs stable under Node 18 LTS.

Regarding Karma and watch mode: The unit tests work fine in headless single-run mode. @chrismayer Could you give us some insights on what is supposed to work in watch mode, since I have no experience with that. See also Stepwise Debugging chapter of the readme.

I will add some comments inline, which are mostly questions. Maybe you have a second look there.

Here is a summarization on the topics we discussed thoughout the previous PRs. I opened up some issues so we don't loose track of those. If I forgot something feel free to remind me :)

• Enforce Vuetify ESLint rules. #322: We could incoporate this in a follow up PR. If you have time to work on it, feel free to do so :)
• Streamline console logging behavior. #323:
• Optimize bundle size #324

[chrismayer]

Could you give us some insights on what is supposed to work in watch mode, since I have no experience with that

No, I have no experience with watch mode / stepwise debug for the unit tests either. For me it was enough to call the test suite most of the times I developed tests. The https://github.com/wegue-oss/wegue/blob/master/test/README.md was introduced by @justb4. Maybe he knows something about it. If it does not work properly with our tools anymore it might be OK to remove and make an issue of re-adding this debugging feature for v2 if someone really needs it.

[fschmenger]

Hi @sronveaux

I had a closer look at the unit tests again, and currently I`m getting this warning:

karma-webpack does not currently support custom entries, if this is something you need,
consider opening an issue.
ignoring attempt to set the entry option...

karma-webpack does not currently support customized filenames via
webpack output.filename, if this is something you need consider opening an issue.
defaulting static/js/[name].js to [name].js.

There is also a more severe bug when moving to OL 6.15.7- see #320.

[sronveaux]

Hi @fschmenger, hi @chrismayer

Thanks for the review and happy to see it is appreciated !

Concerning the Karma watch mode, as explained once, I'm pretty sure all works as expected, you can manage to break it but it comes from an incorrect use of Karma itself. What needs to be done in order to break it in this PR also breaks in current Wegue version, so this is not a regression introduced by this PR...
We just have to keep in mind that running in this mode is an intentional command typed by the user (or an intentional change in the karma.conf.js), so the only thing that can be done here is remove the explanation in the documentation, but nothing hard-coded in Wegue should be problematic for this I think.

@fschmenger concerning the warnings you see in Karma-Webpack, these should be irrelevant in our case as I'll explain just down here. The potential issues they warn about are very specific and would only happen in complex scenarii as you'll certainly understand. Also, given the fact that the latest version of karma-webpack is from February 2021 and nobody requested those addition to the author as stated in the warnings, it really looks like nobody is impacted. Some issues are still opened on their Github repo and discussions are followed so this plugin is not dead so if someone had problems with that, we'd certainly find something but there's nothing.
Why those warnings shouldn't introduce bugs is explain down here:

1. Concerning karma-webpack does not currently support customized filenames: This warning is a nice addition in the new version of karma-webpack V5 which in fact tells us what it has always done behind the scenes silently. If you look at source code of karma-webpack V4, you'll find this. Just that now there is an if warning us that the output.filename won't be taken into account during testing as it was always the case before.

2. Concerning the karma-webpack does not currently support custom entries: Here also, this warning is a kind addition in the new version of karma-webpack V5. It comes from the fact that we have entries automatically defined for us in the Webpack config by Vue which is almost normal as we are in this scenario. (I said almost as our different pages are linked to the same entry point in our case so we should ideally have only one entry, but that's how Vue builds the Webpack config while reading the vue.config.js... that's not our point here.)
   However, for Karma to run properly, it has to build its custom package where the entry point is the test runner and not Wegue itself. You can get a general idea of what's happening here. The idea is that all what is required from tests/unit/index.js and its dependencies (each and every specfile in our case) will be bundled without worrying about what the entry point of the project is.

We could potentially remove those warnings be changing the webpack config manually when it's loaded in tests/karma.conf.js. However, this would be doing what the warning tells us karma-webpack is doing and would, I think, hide the fact that the Webpack config given to Karma is not the one we have written at first. As they are simple additions around how the plugin always worked, I don't think if worths the pain given the fact that if someone is doing very specific things inside it's app, hiding those warnings would hide potential investigation paths to solve their problems.

Unfortunately, I'm almost 100% sure the #320 error is not linked to those warnings and should be addressed apart... I'll comment on this issue as you requested as soon as I'll have time...

At last, concerning the conversations you opened, I'll reply separately as soon as I'll have time to check before telling silly things ;-)

Thanks again and hope this quite lengthy explanation will be useful to all,

Cheers,
Sébastien

[fschmenger]

Hi @sronveaux, thanks once again for the verbose explanation on how the build system behind Karma-Webpack works!

Regarding the warnings: Those are no biggie. I assume that the entrypoint of the application will never be required for the unit tests. So we could just get rid of the warnings as you proposed in your snippet in #320.

Regarding the build error with the new OL (described in #320):
With the help of your hints, I could finally track it down to Webpack Config using the wrong build target for the unit tests. It has been set to node where I it should be set to web to produce a bundle that can run under Chrome Headeless. This is the difference to the production build, which uses browserlist as a target and caused all the issues. See webpack targets
So I fixed it with the following snippet in karma.conf.js:

const webpackConfig = require('../../node_modules/@vue/cli-service/webpack.config.js');
webpackConfig.target = 'web';

To give a brief explanation why it is hitting us on the OL migration:
OL now includes a module called 'web-worker'. This comes in 2 versions, 1 for node /cjs/node.js and another for browser environment /cjs/brower.js, where in the latter case it's simply an alias to Worker. This is probably the first time that the wrong build target mattered, which is apparent from the following warning:

WARNING in ./node_modules/web-worker/cjs/node.js 214:8-20
Critical dependency: the request of a dependency is an expression
 @ ./node_modules/geotiff/dist-module/worker/decoder.js 2:8-40 5:21-27
 @ ./node_modules/geotiff/dist-module/pool.js 49:8-37
 @ ./node_modules/geotiff/dist-module/geotiff.js 5:0-29 732:0-16
 @ ./node_modules/ol/source/GeoTIFF.js 21:0-108 77:25-29 189:18-30 192:18-30 195:18-29
 @ ./node_modules/ol/source.js 10:0-57 10:0-57
 @ ./node_modules/babel-loader/lib/index.js??clonedRuleSet-40.use[0]!./node_modules/@vue/cli-service/node_modules/@vue/vue-loader-v15/lib/index.js??vue-loader-options!./src/components/ol/Map.vue?vue&type=script&lang=js& 22:14-34
 @ ./src/components/ol/Map.vue?vue&type=script&lang=js& 1:0-233 1:249-252 1:254-484 1:254-484
 @ ./src/components/ol/Map.vue 2:0-55 3:0-50 3:0-50 14:2-8
 @ ./src/ sync ^\.\/(?%21main(\.js)?$) ./components/ol/Map.vue ./components/ol/Map
 @ ./tests/unit/index.js 33:19-78

Note: I tried to fix it in vue.config.js chainWebpack, but if I do

if (process.env.NODE_ENV === 'test') {
   config.target = 'web'
}

then it somehow gets rolled back under the hood to node.

All of this results in the following questions:

• Can we somehow consolidate what's in vue.config.js chainWebpack and the (potential) modifications to the config from karma.conf.js ? I think, if possible the webpack configuration for testing should not be scattered over 2 places. So we should either do the fixes mentioned above in chainWebpack (if it can be done), or move everything over to karma.conf.js.
• As you pointed out this is probably not a regression in this PR. Do you mind fixing it in here, since it`s somehow in the scope of the build ecosystem, or should we do it separately?

Greets,
Felix

[fschmenger]

I forgot one thing connected to the same topic: You pointed me to the karma-webpack source. As far as I can see it already should turn splitChunks and runtimeChunk off. You have any idea why it doesn't work when I remove the corresponding lines in chainWebpack?

[sronveaux]

Hi @fschmenger,

Glad that my boring explanations were not useless and helped you find the problem we had here !

For sure, I can try to solve this here as this is the most logical place to do so. I just have very few time for now and will do it as soon as I can. I'll have to delve a bit more inside this and see how we could bring all together cleanly.

This totally makes sense and is unfortunately a big problem with what they've done with Vue-CLI. You don't write the Webpack config directly but have to define things in vue.config.js following their format, then if you have to do more advanced things you have to use chainWebpack but you don't have access to the whole config file directly as it is generated on-the-fly for you. The official documentation says that you require a certain file that dynamically resolves and exports the exact same webpack config used in vue-cli-service commands except it is not in all cases.
I think this comes from the fact that we are in test mode, instead of production mode or development mode and the fact that the recommended test frameworks are now Mochapack and Jest which both are node-oriented and use JSDom instead of a headless browser by default.
I'll make some tests and try to figure out the best way to make this work then come back to you...

Concerning your question on karma-webpack, the link I pointed you to and that you referenced in your question is a link to the karma-webpack V4 codebase which was used in Vue-CLI V4 and is compatible with Webpack4 but not with Webpack5. That's why you have to update it to V5 when you use Vue-CLI V5.
V5 is a complete rewrite of the plugin and the two options you are mentioning are not removed anymore as in V4. They are even set by default in this version.
I can't remember exactly why this was a problem but I clearly remember struggling with it. And I'm not the only one, you can still find some issues in their repository like this one or this one.
I remember I came across a repository of a modified version having only those lines changed. But I didn't want to use a patched version which was not published and would not be maintained afterwards...
It was linked to the official explanation here and the fact that since V5 they decided to generate multiple builds to optimize things. But this proves to be more problematic than positive unfortunately...

Cheers,
Sébastien

[sronveaux]

Hi @fschmenger,

Good news, I had a very short time and managed to correct what you found directly inside the vue.config.js, I managed to make the unit tests run on an updated version of OpenLayers!
I just will have to find the best and cleanest way to do it by delving a little bit more inside Vue-CLI code but this can be be done!
I'll keep you updated as soon as I have time to look more precisely and re-test the whole thing!

[fschmenger]

Hi @sronveaux. Excellent that you are looking into this! There`s no urgency at all so take your time.

Cheers,
Felix

[sronveaux]

Hi @fschmenger,

I had a little time and managed to go to the bottom of it easier than expected. It is indeed directly linked to what I thought at first, the fact that the two recommended test frameworks are node-based and use JSDom by default. In the code, you can even find some clues like this.
When you use Vue-CLI either directly (by calling npm run serve or npm run build) or indirectly by requiring the webpack config file, a lot of things happen under the hood in a way not really documented...
To summarise, the Webpack config is built in two steps, first you have the webpack-chain machinery used to create a raw config, which is mainly defined under the chainWebpack key inside vue.config.js. This raw config is then amended with what you define under the configureWebpack key inside vue.config.js.
These two steps are built from an array of callbacks which are registered from what we define in the vue.config.js file, but can also be registered by any of the installed Vue plugins. And that's where the Mocha plugin registers a callback that uses chainWebpack to add the target="node" key.
As this is done in the webpack-chain step, we can then apply some changes to it from the configureWebpack step afterwards...
I thus propose to do these test config amendments from here, remove the target key as is the case in other modes (web being the default one in Webpack it is not added in development and production modes neither), and potentially the two ones needed to remove the warnings given by karma-webpack if you think it is better to do so. Does it make sense for you?

Cheers,
Sébastien

[fschmenger]

Hi @sronveaux. This makes perfect sense and also explains why my attempt to fix it under chainWebpack did not work as expected. I gave it a quick test run with

configureWebpack: config => {
    // Tweak configuration options for karma unit tests to produce a bundle
    // which can run under chrome headless. Avoid warnings due to custom
    // entries and customized filenames.
    if (process.env.NODE_ENV === 'test') {
      config.devtool = 'eval'
      config.target = 'web'
      config.optimization.runtimeChunk = false
      config.optimization.splitChunks= false
      delete config.entry
      delete config.output.filename
    }
  },

and everything worked out as expected.
Note: There are currently 2 unit tests failing, apparently due to the web bundle doing URL formatting somewhat different as for the node target. But that should be easy to resolve.

Good work!
Greets,
Felix

[sronveaux]

Hi @fschmenger,

Thanks for the comments,

I just took a look to the remaining errors in unit tests and can assure you it has nothing to do with the generated bundle.
It is linked to the formula implemented in OpenLayers used to generate the SCALE parameter if not specified in layerSource construction. Currently used V6.4.3 has this one and since V6.5.0, this one is used which gives different results in lower decimals.
So this is an OpenLayers upgrade bug, not a Vue-CLI one and should be corrected somewhere else, like in #320.

I will certainly take the time to amend this PR next week with all the discussed contents, however I had a simple question/suggestion beforehand:

In what you've written in your comments up here, I would replace the config.target = 'web' line with delete config.target as the other two generated configs (development and production) don't assign a value to it. This would generate the closest possible configurations for the three modes.
If you think explicit is better than implicit, I would suggest to move the line outside the if clause and set the value for allthree configs then... what do you prefer?

Have a nice day,
Sébastien

[fschmenger]

Hi @sronveaux.
You are absolutely right, that the regression in the 2 respective unit tests is in the OL migration. Sorry, I got confused with the test setup. I will re-post it in #320.

Regarding config.target:

• I would really prefer to tweak the config for the test environment only and leave the rest untouched. You really never know what is going to happen in an upcoming vue webpack setup, so this is probably more future proof.
• Whether it's better to set the option to web explicitly or just remove it I can't really tell. My feeling is, that we will never need anything else than a web bundle for the unit tests. While removing the target is more likely to fail, when they change some magic under the hood. But I leave this decision to you.

Cheers,
Felix

[sronveaux]

Hi @fschmenger,

Thanks for the fast reply!
100% agree with you, I'd also prefer to tweak the test config only and certainly, if Wegue stays with Karma (which I personnally think is a good choice), this target will need to be web whatever happens in the future.
Concerning the potential removal of the line or the explicit set, I'll ask the question differently to take the decision then, do you think Wegue would ever need to use anything else than a web bundle? If you think it can, then I'd set the line, if not, I'd remove it. Sure it can break if Webpack team changes something under the hood later, but this change would also break the build itself and would then need this setting to be applied globally...

However, I'm pretty confident this won't change until Webpack 6 which will bring a lot of breaking changes (as always with it) and will eventually need some tweaks here and there... or perhaps will Wegue transit to Vite before that and use Rollup behind the scenes... way too soon to discuss though, that's another history...

[fschmenger]

Hi @sronveaux,
I think there are no short term plans to exchange Karma, as there would be a lot of work involved.

[sronveaux]

Hi @fschmenger,

Found the time before a meeting to amend the PR with all the comments.
This should be it, it will just need to be amended and merged with new additions that would come before being accepted!

Cheers,
Sébastien

[fschmenger]

Hi @sronveaux. Thanks for finalizing this! Looks like you forgot 1 important thing :) See my comments inline.

Cheers,
Felix

[sronveaux]

Hi @fschmenger,

Stupid boy I am, forgot THE important line here! I had it so I suppose something went wrong during the cut / paste operation!
The funniest thing is that I re-tested the whole thing before pushing, but as OpenLayers is still V6.4.3, no problem occured!

I took the opportunity to tune one thing that came through my mind a few days ago and synced following the merge of #325 so this one should now be ready to go and ready to merge in upcoming V2!

[fschmenger]

OK great, looks like we`re finally there.
Thanks @sronveaux for all the work that has gone into this!

[sronveaux]

And thanks for your confidence, patience and time reviewing all those cumulative PRs!

[chrismayer]

Thanks a lot to all, who were involved here.

[sronveaux]

Hi @chrismayer,

So good to see that Wegue V2 will become a reality soon! Thanks for the job!

Just a little warning here, I suppose you only merged PR #320 this morning... my PRs #298, #297 and #312 are marked as merged (because they were inside of #320) however my latest and more up-to-date PR #314 (which I'm replying in) is still marked as open. I suppose this is because some of the commits were not merged inside #320 but cherry-picked.
However, we should take great care and double-verify that all commits of #314 have been taken into account by @spwoodcock in #320 to avoid the latest troubles we've hunted down with @fschmenger...
If all is present inside current Wegue master branch, then we can close this PR...

Thanks again and wish you a pleasant day,

Cheers
Sébastien

[spwoodcock]

You are right @sronveaux, I had merged in up to fc2936a, and then cherry picked f9b0828.

But I'm not sure if the commits after that have been included, my apologies.

One option could be to rebase this branch with master to see which commits are missing after.
Or we could just cherry pick in the commits after f9b0828.

[chrismayer]

Thanks for your important feedback @sronveaux and @spwoodcock. I totally agree that we should carefully check everything is in.

@sronveaux could you please rebase this PR with current master as suggested by @spwoodcock ? Would be really cool and we would easily see, if there is something missing.

[fschmenger]

The problem is, if you rebase everything and push-overwrite, you might lose a lot of information in here like comments to specific check- ins. So I suggest you either setup a new PR which contains everything based on master. The simpler approach could be to just merge the master with this PR again and make it available as a new PR. I unfortunately have no time to study the details at the moment.
Cheers Felix

[chrismayer]

Thanks @fschmenger for sharing your concerns. So just merging in the current master into the base branch of this PR should be fine and less invasive.

@sronveaux Could you do that? If this is not possible from your side I could do this but I would have to do in a new branch since I have no write permissions for your branch.

[sronveaux]

Hi @chrismayer @spwoodcock @fschmenger,

Thanks for your returns on this.

@spwoodcock don't worry, things like this happen, it shouldn't be too hard to fix in the end!
@fschmenger thanks for your comment, I would have avoided to rebase the whole thing too and am happy you confirmed my position,
@chrismayer, I'll take a look at this, either this afternoon or on Friday and keep you updated as soon as its done!

Cheers
Sébastien

[sronveaux]

Hi @chrismayer,

I just merged the current master as expected. I also verified the following:

• No lint errors are present
• The project builds correctly
• All unit tests pass
• Docker image can be built and run flawlessly

That should do it. I just took the opportunity to correct the package-lock.json file which was turned into the V2 format after this PR was released but was still tagged as V1. Wegue was also listed as still being V1.2.
You can easily review it, it's a 3-line change.

Hope this is now ready to go!

Cheers,
Sébastien

[chrismayer]

I just tested this with a recent application based on the current master. Everything looks good, the build works and the test suite passes. Therefore I'll happily merge this.

Thanks again to all who were involved and especially @sronveaux for this last extra step to make this mergeable again.