-
Notifications
You must be signed in to change notification settings - Fork 41
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 v3 template #297
Conversation
…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
…g of eslint to devServer
…port config file deprecations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Wegue app runs for me like expected. Also the built runs like it should.
I am not that much into Webpack, that's why I cannot really give any feedback.
@sronveaux Let me know if there is any specific behavior I shall review
I don't know exactly what happened but this PR is also containing #298. (Transition from V2 to V3 starts in fact at commit ee51e31) @JakobMiksch Concerning Webpack and potential feedback, the idea is indeed to tune the configuration differently if you have some needs on your side. This can be discussed somewhere else later though... |
Hi @sronveaux! The first question is:
Regarding the unit tests:
Some other stuff:
Overall a really nice job and I`m also happy to see that the build output size shrinked quite remarkably (roughly 27 MB previously to 16,6 MB now). Regarding your plans for a vue-cli 4 based version: I have been looking at the recent vue documentation and they now recommend Vite as their preferred toolchain (https://vuejs.org/guide/scaling-up/tooling.html). Do you have any experience with Vite and would a transition be feasible for Wegue? I guess, as it relies on browser support for native ES Modules, we might have to drop support for legacy browsers (i.e. I have no clue how much this will affect some mobile devices). If you have any experience with Vite, I`m glad to hear your opinion. A final word on the integration process: I think, we cannot do the whole transition before going to Wegue 2.0, as some dependencies now upgraded to a new major version. So my feeling is, that we will break something for existing Wegue applications. This may be a while, so I propose you don`t bother with merging against the master until we are nearly there. Alright, that`s it from me |
*/ | ||
module.exports = { | ||
assetsDir: 'static', | ||
runtimeCompiler: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the runtime compiler? At least for my tests, the application seems to build and run fine without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends how you define your Vue components. I'm almost sure that the demo app doesn't need it as you said.
It was added to reduce the risk to break some apps as the documentation states that it only incurs a supplementary 10kb payload for the entire app.
I don't remember exactly but I'm wondering if some unit tests were not dependent on it with some mount commands redefining the template of components. I'm not 100% sure though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far I had no problem here with unit tests or the custom app I tested. I think the default templates turn it off, because it imposes a runtime overhead. Could you figure out, whether a runtime compile is supported by the current Wegue toolchain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concerning this point, I made some more researches. It's true that Vue CLI since V3 turns this value as false by default.
However, as stated above, this was set to reduce the risk to break some apps as the compiler IS currently bundled in the latest Wegue version.
This was also the default value in the template of Vue CLI V2.
So to answer your question, yes, current Wegue toolchain supports compilation at runtime and could potentially use it.
On the other side, I agree with you 100%, this shouldn't be used in default Wegue app and would only be used in very peculiar situations. This compiler is in fact automatically called when your code contains components which were not written in .vue files and were not processed by webpack beforehand. This can be useful if someone uses a component with dynamic templates for example.
I came on this SO discussion where all I could write here is already stated very clearly.
To summarize, I would, as you suggested, have removed it personally but didn't want to include a potential breaking change. Perhaps removing it but mentioning it later in a migration note or something could be a good idea... just tell me what you want me to do and if I should do it now or in the next transition to V4...
Hi @fschmenger Many thanks for the complete review and all the wise comments you made. For sure such a big change must be tested on multiple bigger apps and we'll certainly have some other things to tune here and there... Totally agree that vue-cli3 should be the minimum. I would even say that vue-cli4 should be the minimum as it's still updated where cli-3 isn't anymore. Here are answers to your questions:
I also used v3.12.1 however I used it as a source for comparison but then installed everything inside the current Wegue version, not building it from scratch, which could potentially give some dependencies diffs. Don't hesitate to ask if one seems problematic and I'll try to follow the history and find where, why and how it was introduced that way.
Seems perfect for me, I have nothing against Karma, just that it isn't installed at all anymore since vue-cli3 so I had to reintroduce everything from scratch. As I didn't know if it was more important for you to stick with Karma or to stay in line with what vue-cli recommends, I momentarily kept the two side by side so you could also give it a try if you wish. I think the native cli-3 solution will have to stay installed but removing all entrypoints will be done easily.
Exactly as you said, I renamed test to tests (ac32e21) to conform to the new vue-cli template. I certainly have missed something in the .gitignore and will correct it.
Yes, in fact I made this work at a time when the workshop wasn't in the repo and used tag 1.0 as a reference. That's why I haven't reviewed the workshop part at all. This will be done.
I must admit I never tried to continue working if the port was already used neither with current Wegue version nor with the version in this PR. I'll take a look at this when I'll have a little time for it and keep you updated.
Those will then be answered in their own comments
Well this is a tricky question here. I must admit I don't have that much experience with Vite but whether Wegue should transition to it really depends on what you're planning to do later. Take the following as my own thinkings, not as wise advices: At first, if you asked me the same question in January when I first discovered Wegue, I would have said certainly not now as Vite was fully developed aiming Vue3. To get compatible with Vue2 you had to install a third-party plugin at that time and it seems Vuetify was not that happy to integrate neither. Their website says that Vuetify3 will be compatible with Vite but Vuetify3 is also aimed towards Vue3... you can certainly see the problem here... With the little experience I have with Vite, configuration is quite close to what is done in vue-cli since V3 so the transition could potentially be tried without too much work. What to expect afterwards is not as clear however. You're totally right that support for legacy browsers has to be dropped but, normally, only during development, which is not a problem I think. When going to production, Vite is in fact using ESbuild to transpile and minify javascript code then it's using a modified version of Rollup instead of Webpack to bundle everything. There exists a plugin (@vitejs/plugin-legacy) which uses Babel to create a polyfill-js-chunk which is loaded only by the browsers which are not ESM compatible. So this could potentially be working as expected on legacy hardware and/or software. But as I said, I don't have enough experience with it to assure that it will work flawlessly... As stated earlier, there also is a risk that the whole unit tests harness has to be reviewed. A quick browse through the Karma-Vite plugin source code seems to be less problematic than what is done by Karma-Webpack5 (which is my last barrier to finish the transition of Wegue to vue-cli5) and I'm quite optimistic here that this can work as expected without too much extra work.
Thanks for telling me, don't hesitate to contact me when you think the time has come. Do you have an idea of the timing for this ? Many thanks again for the reply and interest in this PR, |
Hi @sronveaux!
Honestly, I have no idea on the exact timeline. I`m pretty sure the move wont happen before next year, as Wegue is mostly developed by people in their spare time. If you proceed over here I can review it from time to time.
Thanks for all the valuable clarifications on Vite. Cheers, |
Thanks @sronveaux for bringing this up. This is huge IMHO and will bring the project a big step forward. Also thanks @fschmenger for your detailed reviews and your valuable feedback. Great! As already said in some offline-mail-talk with @sronveaux I really second this initiative. Unfortunately I am not a webpack et al. expert but my input could be to test this with some existing Wegue apps. As a first step I'd favor the following having in Wegue:
Switching to Vite could be done afterwards and should be discussed beforehand. Maybe related to a possible future upgrade to Vue3. So changesets will be not too big and manageable. Big doing everything at once does not seem to be an effective way. Therefore I really appreciate the way @sronveaux upgraded in increments by separate PRs (cli v2 -> cli v3, ...) |
Thanks @chrismayer and @fschmenger for the positive commentaries, unfortunately I had a lot of urgent and important stuff to do last month so I had no time to answer and continue on the subject... Now this is behind and I should have a small air bubble to tackle those remaining glitches. What I propose is to amend this PR with all what was discussed with @fschmenger so we can resolve the open conversations. Afterwards, I'll post the PR to upgrade CLI V3 -> CLI V4. Do you prefer that I open a new PR from there or to nest those steps in this one ? Depending on the time I have after that, I still hope to get the transition to CLI V5 to a fully working state. It is already, the remaining problems are linked to unit tests. You can run them only once and Karma can't run in watch mode for now. Other than that, it is functional since end of July... @chrismayer, for sure, with such an update, having the opportunity to try different apps will be the trigger to accept or postpone the merge in the end I think. If you see things to change during those tests, don't hesitate to ask ! Cheers, |
…ack scripts from package.json and README.md
Hi @fschmenger, Now (almost) all what was discussed is hopefully corrected. Here are answers to your inline questions down below. I'll reply to conversations in their respective windows up here.
This was done in b78606d
Both done in e8f1475
As explained, I didn't look at the workshop at all which didn't exist when I started to code all this. It was all reviewed and corrected by edf6427
I could easily reproduce and confirm there is a problem here. It is solved when using Vue-CLI V4 though. I rapidly tried to find the reason from source code but didn't find anything simple. I can't tell now if it's linked to a dependency or directly in the Vue-CLI code as Webpack-dev-server reports it will run on, for example, port 8082 but port 8081 is still injected in the client code bundled by Webpack. However, as this will be solved with the next PR I'll post, I would suggest to stop researches here on this point if it's O.K. for you... The remaining points are answered in their respective conversations. Don't hesitate to ask if there's something else, as said before, as soon as this will be O.K. for you, I'll add the transition to Vue-CLI V4. Just tell me if you prefer another PR or if you prefer that I amend this one. Cheers, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sronveaux,
many thanks for implmenting all the requested changes! I could see everything being addressed. Good job!
Regarding the runtime compiler:
Personally I would opt to turn it off, because
- It`s turned off in the cli template by default
- It decreases the bundle size by 200kB
- Runtime template compilation is a more advanced feature, most likely in use by an experienced Vue developer only. He may easily be aware of the setting and tweak it back if needed.
Another question I would bring up, is whether we really need production source maps - as they currently make up for roughly 7MB (almost half the bundle size).
I'm not the only one to make a ruling on the 2 topics above, so I'm curious what others have to say about it!? .. @chrismayer @JakobMiksch
Anyway, we don't have to address this right now. Maybe it's a good idea to open up a dedicated issue for that (e.g. optimize bundle size
) and we discuss before moving to v2?
Cheers,
Felix
A final note: IMO this is as good as it can get. I don't approve it yet, so it doesn`t get merged accidentally before we're ready for the v2 migration. |
Hi @chrismayer, hi @JakobMiksch, Just a little reminder, don't forget to tell me :
I hope to get a little time to work on this during next weeks, so if you tell me what you'd prefer, I'll have evrything in hand to make this upgrade as soon as I'll have the time to work on it ! Cheers, |
Hi @sronveaux! Good to hear your plans on working out the v4 upgrade. This is very much appreciated! Regarding
The preferred approach would be to open a new PR and base it on this one. If this is inconvenient for one or the other reason let us know.
and also my suggestion
I would be also interested to hear other opinions @chrismayer @JakobMiksch. Anyway if I´m not mistaken these should be 2 flags to tweak in the configuration - So don`t feel blocked by this if you want to proceed. Thanks for the good work, looking forward to v4. |
Here's a second PR linked to the upgrade of Vue-CLI and Webpack (#94)
Here things become more serious as breaking changes were introduced in the way Vue-CLI templates were designed, not to mention the new plugins architecture...
The main point of this upgrade is to transition from
Webpack 3
toWebpack 4
.The project architecture was completely changed with this version. The main changes are the following ones:
vue.config.js
filepackage.json
(.browserlistrc
).babelrc
tobabel.config.js
and.postcssrc.js
topostcss.config.js
)static
folder was renamedpublic
.index.html
andembedded.html
files were moved inside thispublic
directoryNPM scripts changed with this release.
npm run dev
is nownpm run serve
.I also changed the linting NPM scripts as the new default script fixes all the errors by default. I preferred to stay as before (
npm run lint
just checks the code and reports errors) but added a newnpm run lint:fix
command to automatically fix linting errors.Eventually, the biggest change with Vue-CLI V3 is linked to unit tests. They are still using
Mocha
andChai
but droppedKarma
as a test launcher. They rely onmocha-webpack
which itself usesJSDom
instead of a real headless browser. This will be the hardest part to transition as a lot of tests start to fail when usingJSDom
because of functions no properly emulated (at least with the versions included by Vue-CLI).For now I reintegrated
Karma 6.4
inside the project and linked it to a new NPM scriptnpm run test:karma
.The
npm run test
usesmocha-webpack
but doesn't work at all in this version as I haven't included the init script. As lots of test are broken with it, I thought it would be useless... at least for now.I already have the next PR ready to transition to the latest Vue-CLI V4 template (V4.5.19 out on 28th of June 2022). I'd like to have some feedback before posting though as I want to know if there is something to change beforehand.
Hope all this work will help...