-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[Vue] Replace Webpack with Vite #22356
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
base: main
Are you sure you want to change the base?
Conversation
01011b3 to
398cdae
Compare
|
I'll need to fix tests, but I think this is OK for a review. I also need to figure out HMR functionality so changes in the code can be instantly seen in the browser. With Webpack, I used the "writeToDisk" options for the development server to avoid directly using an external server. With Vite, this isn't an option. In the plugin I've been using Vite with, I have it check if GLPI is in development mode and if the server is running (using fsockopen) and switch out the JS path with the one for the dev server. This may be a necessary evil with the current architecture, but maybe someone has other ideas or maybe I missed something with Vite to have a similar functionality as Webpack. |
To be sure about my understanding, Vite, instead of webpack, could take in charge ESM but not plain JS ? |
It may just be a lack of my own understanding, but the issue I faced when testing Vite with the |
AdrienClairembault
left a comment
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.
Ok for me but it should target main to be safer IMO (especially since the vite dependency is a beta release).
|
I'm not sure we should (once again) rely on an unstable lib - even in main. |
|
The stable version should be available on Q1 2026 so its fine for main I think, it isn't like tabler where we had no visibility on the release date. |
That's fair, but in this case it is a dev lib for a handful of features instead of one that affects what the user sees. I don't know when it will come out of beta, but I don't think it will be many months/years. |
|
Vite is by far currently the best frontend toolkit available For safety, I agree we may include the changes in main, but later if everything goes well (and the stable release out), we may include this event in 10.0/bf, probably. |
Note that vite itself is not in beta and has many stable release, it is just the specific version required in the dependencies here that target a beta version. I don't think this is a bad idea to target this beta so we are already "up to date" and avoid migrating to it in a few month, as long as we use it only for main. |
|
OK for me, in main for now - with the idea to backport in a few weeks, when lib will be stable, if there is no problem on main with it. |
src/Html.php
Outdated
| $tpl_vars['js_modules'][] = ['path' => 'build/vue/app.js']; | ||
| // Vue entrypoint needs no version param because the chunks already have the hash in the name | ||
| // The entrypoint itself should rarely change but devs could also always use HMR | ||
| $tpl_vars['js_modules'][] = ['path' => 'build/vue/app.js', 'options' => ['no_version' => 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.
We need a version parameter at least in production environments, to be sure that the file from a previous version is never used when extensive cache headers are forced by web server rules.
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.
OK. I'll need to figure out why it was being imported twice, once with the version parameter and then later without it. It was causing some odd errors.
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.
I handled cache busting differently here. Vite now puts the hash in the file name of the "app.js" as well, and GLPI simply refers to the manifest to determine the path to use.
Ok for me too. |
df0fad5 to
8c93a60
Compare
|
I have some doubts about the requirements of the entire HEADER in built JS files, especially when they are minified. They are present now, but it may not be on the first line all the time like the checker expects, but it seems like the correct behavior since the GLPI code is not always going to come first. I did check if a HTML comment with the copyright header in the Vue file would be preserved and it was not. It was preserved if added to the script tag in the file although it could get annoying if it is the entire header. Is this not sufficient as a header? |
|
cf gnu FAQ: https://www.gnu.org/licenses/gpl-howto.en.html#license-notices
I'm not a lawyer, and I don't have anyone to explore the subject in depth. On build artifacts, I guess it doesn't matter too much, but again I'm not 100% sure about that |
|
To expands a bit, this practice seems a bit a relic of the past, and more recent projects don't use preamble in their files, also ones with GPL licenses. |
|
We could have a simplified version using
I even saw recently that symfony also reduced it's header licence to just a see LICENCE file (but probably too extreme and not following any standard) edit: just saw last comment from @orthagh after sending my message. I'll let it here as it's always a good thing to know about the SPDX-License-Identifier |
AFAIK minified files cannot be considered "source" since GPL defines source as the preferred form for modification which bundled, and way more specifically any minified files, are not. I do think they need added to the Vue files though in some form or another. |
So ok for that, let's drop this part |
Checklist before requesting a review
Description
Discussed in #22330. Completely replacing Webpack with Vite (made by the same author as Vue) is not feasible at the moment for the general JS used by GLPI because of the mix of plain JS and ESM code, and Vite works natively with ESM code. However, Vue bundling has always been separate and ESM.
Going from Webpack to Vite results in builds that are 2.5x faster locally.
This also unlocks useful tools in the ecosystem like Vitest which can replace Jest (which barely has ESM support at all) and Cypress with a "browser mode" which uses Playwright. Vitest also has the ability to run only changed tests/tests related to changed sources, which may be able to reduce CI times later on when there is more separation between server and frontend code.