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

Update to last Vue CLI v4 template #312

Merged
merged 63 commits into from
Jun 26, 2023
Merged

Conversation

sronveaux
Copy link
Collaborator

@sronveaux sronveaux commented Dec 20, 2022

Hello,

Eventually I had the time to create this PR we talked about for months now! Hope this will work flawlessly...

This PR is way smaller than Vue CLI3 and should be way shorter to review too... I still hope to find time to make Wegue fully functional with Vue-CLI V5 in a not too distant future... it already is, only unit tests are having some troubles...

This one is based on the last Vue CLI V4 template (4.5.19).
New Wegue functionalities developed since this migration was done have been merged before applying. So this PR should be up to date for now...

To stay in tune with one of our previous discussions, minimal versions of node and npm have been set according to the official Vue documentation where it is stated that minimal recommended version is V10+. However I took

"engines": {
  "node": ">= 10.13.0",
  "npm": ">= 6.4.1"
}

to use the first LTS version among V10 ones.

We also discussed an issue with the portfinder mechanism:

  • For some reason the portfinder mechanism of the dev-server runs into a problem when port 8081 is already in use. It correctly finds a free port (8082 for me) and spawns the service there, but some internal part still adresses 8081, so I see a lot of GET http://localhost:8081/sockjs-node/info?t=1661260791345 net::ERR_FAILED 404 in the logs. I wonder if something can be done about that issue?

it seems to be working as expected with the underlying updates that this PR makes.

If this is of interest for you and to make a followup of my comment, I can try and see if I can easily add the official Vuetify plugins in order to have access to Vuetify ESLint rules. I already did it with ESLint plugin only, not with the full plugins pack. Just tell me if this is of interest and I'll give it a try as soon as I'll have a little time...

Merry X-Mas to the whole team!

Cheers
Sébastien

…mplate. The biggest difference is updating to Webpack 3.x and using Webpack-dev-server instead of Express
…peerDeps and newer versions of NPM. This was already present before the latest updates though
… Changed their content to conform to the new template
…js@2 because Vue-CLI V3 is incompatible with core-js@3
.eslintrc.js Outdated
Comment on lines 12 to 13
// 'no-console': process.env.NODE_ENV === 'production' ? 'warn' : 'off',
'no-debugger': process.env.NODE_ENV === 'production' ? 'warn' : 'off',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I would prefer to raise a 'no-debugger' error for production environments.

And as a future proposal - probably as part of a seperate PR:
Maybe we could do something about the 'no-console' setting. E.g. we could pool all wegue logging operations into a logger module and then use the eslint-disable directive there. So warnings could be generally enabled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem to revert the change, simply, that's not something that I personally changed, this is what is present in Vue-CLI V4 and Vue-CLI V5 templates.

Your future proposal is intersting, it would be nice to enable this rule in the end for sure!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted to raise error if debugger is used while building for production

@fschmenger
Copy link
Collaborator

Hi @sronveaux.

I didn`t find time to give this a test yet - hopefully will have by the beginning of next year.
From looking over the code there are suprisingly little changes.

Regarding eslint-plugin-vuetify:
In general I appreciate the idea. Is installing the plugin an option when creating an app via cli-4? If so it might go in here.
But keep in mind that this will probably impose some more code changes. So when in doubt put this in a separate PR.

The choice of "node": ">= 10.13.0" makes perfectly sense to me.

Anyway, thanks a lot for providing this so quickly! I wish you a nice and relaxing christmas time.
Cheers,
Felix

@sronveaux
Copy link
Collaborator Author

Hi @fschmenger,

Thanks for the fast reply, don't worry and take your time...
As you said, there is nothing big here, the transition from V2 to V3 was huge compared to this...

Regarding eslint-plugin-vuetify:
In general I appreciate the idea. Is installing the plugin an option when creating an app via cli-4? If so it might go in here.
But keep in mind that this will probably impose some more code changes. So when in doubt put this in a separate PR.

Installing the plugin is not an option when creating an app but almost. When the new app is created, you start by running vue add vuetify which runs you through another wizard (such as when creating a pure Vue app) and changes the package.json and main.js files accordingly. So it's something you should do directly as another app template is applied at that time.
The idea would be to stick with the Vuetify version currently used by Wegue but adding some Vue-CLI possibilities. This can easily go in a separate PR by the way...
I'll give it a try when I'll have some time next year and will share if something useful gets out of it...

Cheers,
Sébastien

@fschmenger
Copy link
Collaborator

Hi @sronveaux.
I gave it a test this morning in a larger custom app and everything works out fine.
As you pointed out, they also fixed up portfinder, so no more trouble there.

One last finding:
From looking at a template v4.5.19 app I can see that the vue versions in package.json are assigned with a ^ , so a user could possibly choose to go with vue-v2-latest. So I suggest we do the same here:

"vue": "^2.6.11"
"vue-template-compiler": "^2.6.11"

I haven`t installed vuetify in the template app, but might be the same thing. Could you check that?

Anyway I hope you had some relaxing christmas days.
A happy new year to you!

@fschmenger
Copy link
Collaborator

Ooops, I just discovered that the docker build is failing at npm run build stage. The node version running inside the lts-alpine image is currently 18.12.1 - see dockerhub and nodejs docker source.

For me it currently results in this build error:

#12 1.417 > vue-cli-service build
#12 1.417 #12 2.198 
#12 2.199 -  Building for production...#12 3.669 Error: error:0308010C:digital envelope routines::unsupported
#12 3.669     at new Hash (node:internal/crypto/hash:71:19)
#12 3.669     at Object.createHash (node:crypto:133:10)
#12 3.669     at module.exports (/app/node_modules/webpack/lib/util/createHash.js:135:53)
#12 3.669     at NormalModule._initBuildHash (/app/node_modules/webpack/lib/NormalModule.js:417:16)
#12 3.669     at handleParseError (/app/node_modules/webpack/lib/NormalModule.js:471:10)
#12 3.669     at /app/node_modules/webpack/lib/NormalModule.js:503:5
#12 3.669     at /app/node_modules/webpack/lib/NormalModule.js:358:12
#12 3.669     at /app/node_modules/loader-runner/lib/LoaderRunner.js:373:3
#12 3.669     at iterateNormalLoaders (/app/node_modules/loader-runner/lib/LoaderRunner.js:214:10)
#12 3.669     at iterateNormalLoaders (/app/node_modules/loader-runner/lib/LoaderRunner.js:221:10)
#12 3.669     at /app/node_modules/loader-runner/lib/LoaderRunner.js:236:3
#12 3.669     at runSyncOrAsync (/app/node_modules/loader-runner/lib/LoaderRunner.js:130:11)
#12 3.669     at iterateNormalLoaders (/app/node_modules/loader-runner/lib/LoaderRunner.js:232:2)
#12 3.669     at Array.<anonymous> (/app/node_modules/loader-runner/lib/LoaderRunner.js:205:4)
#12 3.669     at Storage.finished (/app/node_modules/webpack/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:55:16)
#12 3.669     at /app/node_modules/webpack/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:91:9
#12 3.672 Error: error:0308010C:digital envelope routines::unsupported
#12 3.672     at new Hash (node:internal/crypto/hash:71:19)
#12 3.672     at Object.createHash (node:crypto:133:10)
#12 3.672     at module.exports (/app/node_modules/webpack/lib/util/createHash.js:135:53)
#12 3.672     at NormalModule._initBuildHash (/app/node_modules/webpack/lib/NormalModule.js:417:16)
#12 3.672     at handleParseError (/app/node_modules/webpack/lib/NormalModule.js:471:10)
#12 3.672     at /app/node_modules/webpack/lib/NormalModule.js:503:5
#12 3.672     at /app/node_modules/webpack/lib/NormalModule.js:358:12
#12 3.672     at /app/node_modules/loader-runner/lib/LoaderRunner.js:373:3
#12 3.672     at iterateNormalLoaders (/app/node_modules/loader-runner/lib/LoaderRunner.js:214:10)
#12 3.672     at iterateNormalLoaders (/app/node_modules/loader-runner/lib/LoaderRunner.js:221:10)
#12 3.672     at /app/node_modules/loader-runner/lib/LoaderRunner.js:236:3
#12 3.672     at runSyncOrAsync (/app/node_modules/loader-runner/lib/LoaderRunner.js:130:11)
#12 3.672     at iterateNormalLoaders (/app/node_modules/loader-runner/lib/LoaderRunner.js:232:2)
#12 3.672     at Array.<anonymous> (/app/node_modules/loader-runner/lib/LoaderRunner.js:205:4)
#12 3.672     at Storage.finished (/app/node_modules/webpack/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:55:16)
#12 3.672     at /app/node_modules/webpack/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:91:9#12 6.801 /app/node_modules/loader-runner/lib/LoaderRunner.js:114
#12 6.801 throw e;
#12 6.801 ^
#12 6.801 
#12 6.801 Error: error:0308010C:digital envelope routines::unsupported
#12 6.801     at new Hash (node:internal/crypto/hash:71:19)
#12 6.801     at Object.createHash (node:crypto:133:10)
#12 6.801     at module.exports (/app/node_modules/webpack/lib/util/createHash.js:135:53)
#12 6.801     at NormalModule._initBuildHash (/app/node_modules/webpack/lib/NormalModule.js:417:16)
#12 6.801     at handleParseError (/app/node_modules/webpack/lib/NormalModule.js:471:10)
#12 6.801     at /app/node_modules/webpack/lib/NormalModule.js:503:5
#12 6.801     at /app/node_modules/webpack/lib/NormalModule.js:358:12
#12 6.801     at /app/node_modules/loader-runner/lib/LoaderRunner.js:373:3#12 6.801     at iterateNormalLoaders (/app/node_modules/loader-runner/lib/LoaderRunner.js:214:10)
#12 6.801     at iterateNormalLoaders (/app/node_modules/loader-runner/lib/LoaderRunner.js:221:10)
#12 6.801     at /app/node_modules/loader-runner/lib/LoaderRunner.js:236:3
#12 6.801     at context.callback (/app/node_modules/loader-runner/lib/LoaderRunner.js:111:13)
#12 6.801     at /app/node_modules/cache-loader/dist/index.js:147:7
#12 6.801     at /app/node_modules/graceful-fs/graceful-fs.js:61:14
#12 6.801     at FSReqCallback.oncomplete (node:fs:197:23) {
#12 6.801   opensslErrorStack: [ 'error:03000086:digital envelope routines::initialization error' ],
#12 6.801   library: 'digital envelope routines',
#12 6.801   reason: 'unsupported',
#12 6.801   code: 'ERR_OSSL_EVP_UNSUPPORTED'
#12 6.801 }
#12 6.801 
#12 6.801 Node.js v18.12.1

Here is some background I found on Stack overflow.
The cause seems to be some OpenSSL security fixes in recent node versions and webpack using OpenSSL to create md4 hashes. We don`t run into this in the current Wegue master branch.

Could you have a look into that too? Maybe something along the proposed solutions e.g. using output.hashFunction = 'xxhash64' works for us ?!

@sronveaux
Copy link
Collaborator Author

Hi @fschmenger and thanks for the review !

Christmas break was very resourcing, hope it was great for you too! All the best for 2023 for you and your family, and to the rest of the team too!

I took the time to look at the two things you mentioned up there and here is what I arrived to:

  1. Concerning the tagging of Vue and Vuetify versions:

You're true that the official Vue template tags Vue as ^2.6.11. I tried to install Vuetify from their latest template updated in September 2022 and there they tag Vuetify as ^2.6.0.
In fact, in current Wegue version, Vue and Vuetify are pinned to fixed versions which was not the case in the Vue V2 template. As this was a deliberate choice at a given time to do it this way I prefered to let it untouched for now...

For sure I can easily change this but it all depends how @chrismayer feels about #295. Should I change it in this PR or should it be done in #295 ?

I'm planning to get back on the upgrade to Vue-CLI V5 soon and hope to get it working flawlessly. In this potential upgrade, I'll have to upgrade Vue to 2.6.14 by the way so what I'd propose for the current PR is to go for

"vue": "~2.6.11"
"vue-template-compiler": "~2.6.11"
"vuetify": "~2.2.18"

That way it shouldn't break anything as compared to what was experienced with #295. I have the feeling there shouldn't be any problem when upgrading Vue to 2.7 but Vuetify will not be as easy...

  1. Concerning the unsupported digital envelope routines:

Gosh I missed this one... to be honest I don't use Docker myself which explains why I missed it, however this was induced by the fact that Node 18 is now LTS and so was upgraded in those Docker images. I could easily reproduce the bug simply by upgrading Node on my computer.
As you saw, this is linked to the fact that Node is since v17 using an OpenSSL version where MD4 was removed... and this MD4 implementation was used by Webpack.
Good news is that it was fixed in Webpack5, bad news is we are here on Webpack4...

The idea of using output.hashFunction = 'xxhash64' doesn't work as simple in our case as the MD4 hash is also used by some Webpack plugins underneath. So adding this line makes the process go further but the same error fires again multiple times during the build process...

I have to mention that I tested with the update to Vue-CLI V5 (and so with Webpack5) and there the error is gone... one more reason for me to try again to finish this upgrade!

As this will be fixed if I manage to make unit tests working in Vue-CLI V5, I went on with another easier and direct fix for now. This consists in using NODE_OPTIONS=--openssl-legacy-provider inside the npm scripts involving Webpack. This reintroduces the security hole which was fixed in Node v17 but I think I shouldn't lose too much time on this for now as this will perhaps be a temporary patch before the leap to Webpack5...

Just tell me what you think about those two points and I'll amend the PR accordingly !

Cheers,
Sebastien

@fschmenger
Copy link
Collaborator

Hi @sronveaux,
thanks for looking into this again.

Regarding the vue/ vuetify versions you are absolutely right. I just found this when comparing the code against the template generated definitions. Since we already figured out in #295, that some of the code isn't compatible, I think you can just leave it as is. We can look into this in the process of a v5 template again. Since all of this will be part of Wegue v2 some smaller breaking changes are no biggie.

The second point is a bit unfortunate. I agree that we should rather sit on the upcoming v5 version then. If this is our best bet, then I'm fine to set the proposed NODE_OPTIONS=--openssl-legacy-provider workaround for the time being.

Cheers,
Felix

@sronveaux
Copy link
Collaborator Author

Hi @fschmenger @chrismayer and @JakobMiksch,

I don't know if this is the best place to discuss this...

I took back my tests of upgrading Wegue to Vue-CLI V5 which was 99% done but had a bug when using Karma to run unit tests.
In fact, it was difficult to make it work when I first tried in July 2022 (had to tweak the Webpack configuration disabling some optimization for the tests to run fine) and I think that those difficulties blinded me and made me think there were other problems where in fact there weren't...

At that time, I arrived at the point where I could run the unit tests but only in single-run mode. There were some problems when running in watch mode and attaching a browser, using the debug functionality of Karma etc...

Now that I try this again with a fresh mind, I think that my lack of practice with Karma was the only problem. When you run it in watch mode and attach a headless browser + your browser and run the debug window all at the same time then make some changes in the code to make the whole process run again, you experience problems even on a simple fresh project. It seems you have to close the debug window when triggering a compile/test cycle THEN reopen it if you need afterwards...

That's why I open this discussion here and ask to all of you who certainly have more practice of Karma than me, does what I've written up here make sense, do you sometimes use Karma in watch mode and use theses debug functionalities, etc... and do you sometimes experience difficulties while doing so?

Depending on your comments and feelings, I think I can prepare the long-awaited PR to upgrade Wegue to Vue-CLI V5 which is perhaps not having problems in the end...

I just have a (real) problem left, which I discovered is in fact also present in the upgrades to Vue-CLI V3 and V4... code coverage doesn't take .vue files into account anymore, only .js files. It seems to be a known problem not that easy to solve but I'll try to arrange something here...

Thanks in advance for your comments and all the best to the team

Cheers,
Sebastien

@sronveaux
Copy link
Collaborator Author

Just a word to let you know that I resolved the coverage bug described above. So I amended the PR with this and the workaround needed to function properly with recent Node.js versions...

@fschmenger
Copy link
Collaborator

Hi @sronveaux,
thanks for finalizing this one! I never noticed there is a bug in the reported coverage, good job figuring this one out.

Regarding the browser debugging functionality I never tried anything with the unit tests apart from the headless single-run functionality. However I can point you to the Stepwise Debugging chapter of the readme, so I assume it is supposed to work. Maybe @chrismayer or @JakobMiksch can provide you with more information on that.
If the Karma tests is the only remaining problem, then I propose you provide the v5 version in a separate PR and we can discuss related stuff there!?

Cheers,
Felix

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

Successfully merging this pull request may close these issues.

3 participants