-
Notifications
You must be signed in to change notification settings - Fork 899
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
RUN-2221: Update webpack 4 to 5 #8968
Conversation
rundeckapp/grails-spa/packages/ui-trellis/src/library/utilities/uivi18n.ts
Outdated
Show resolved
Hide resolved
20dbb2d
to
ee370c9
Compare
3af4d6b
to
6bba243
Compare
@@ -82,7 +82,6 @@ | |||
|
|||
<asset:javascript src="framework/editProject.js"/> | |||
<asset:javascript src="static/pages/project-config.js" defer="defer" /> | |||
<asset:stylesheet href="static/css/pages/project-config.css" /> |
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.
This file isn't being created, as the respective components do not have any css to be imported.
@@ -34,7 +34,7 @@ | |||
kind: AuthConstants.TYPE_EVENT | |||
)}"/> | |||
|
|||
<asset:javascript src="static/css/pages/command.css"/> | |||
<asset:stylesheet src="static/css/pages/command.css"/> |
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.
This was causing a 404 as the generated import had the wrong path and extension
@@ -90,7 +90,6 @@ | |||
<asset:javascript src="leavePageConfirm.js"/> | |||
<asset:javascript src="framework/editProject.js"/> | |||
<asset:javascript src="static/pages/project-config.js" defer="defer" /> | |||
<asset:stylesheet href="static/css/pages/project-config.css" /> |
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.
This file isn't being created, as the respective components do not have any css to be imported.
<!-- VUE CSS MODULES --> | ||
<asset:stylesheet href="static/css/components/version-notification.css"/> | ||
<!-- /VUE CSS MODULES --> |
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.
This file isn't being created. In fact the components in the folder version-notification do not seem to be used anywhere.
<asset:javascript src="static/components/first-run.js"/> | ||
<asset:javascript src="static/components/version-notification.js"/> |
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.
This file isn't being created. In fact the components in the folder version-notification do not seem to be used anywhere.
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.
seems so, let's do a separate PR to remove that code since it isn't used
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.
Okay, PR for removal: #9135
@@ -462,9 +459,7 @@ | |||
</div> | |||
<!-- VUE JS MODULES --> | |||
<asset:stylesheet href="static/css/pages/home.css"/> | |||
<asset:stylesheet href="static/css/components/first-run.css"/> |
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.
This file isn't being created, but the import isn't needed as the CSS required by its components is included in the chunk-common.css
@@ -63,7 +63,6 @@ | |||
<asset:javascript src="menu/home.js"/> | |||
|
|||
<!-- VUE CSS MODULES --> | |||
<asset:stylesheet href="static/css/components/version-notification.css"/> |
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.
This file isn't being created. In fact the components in the folder version-notification do not seem to be used anywhere.
@@ -80,7 +79,7 @@ | |||
<div id="layoutBody"> | |||
<div class="vue-ui-socket"> | |||
<g:set var="createProjectAllowed" | |||
value="${auth.resourceAllowedTest(action: AuthConstants.ACTION_CREATE, type: AuthConstants.TYPE_PROJECT, context: AuthConstants.CTX_APPLICATION)}"/> | |||
value="${auth.resourceAllowedTest(action: [AuthConstants.ACTION_CREATE], type: AuthConstants.TYPE_PROJECT, context: AuthConstants.CTX_APPLICATION, has: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.
home.next is behind a feature flag but noticed that this line wasn't 100% its equivalent in home.gsp
@@ -104,7 +103,6 @@ | |||
</div> | |||
<!-- VUE JS MODULES --> | |||
<asset:stylesheet href="static/css/pages/home.css"/> | |||
<asset:javascript src="static/components/version-notification.js"/> |
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.
This file isn't being created. In fact the components in the folder version-notification do not seem to be used anywhere.
@@ -1,3 +1,3 @@ | |||
VUE_APP_OUTPUT_DIR=../../../grails-app/assets/provided/static | |||
VUE_APP_DEVTOOL=eval-cheap-module-source-map | |||
VUE_APP_DEVTOOL=eval-cheap-source-map |
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.
eval-cheap-source-map has better performance than eval-cheap-module-source-map. Open to discussion if team would like to try faster modes:
globals: { | ||
"ts-jest": { | ||
tsconfig: "tsconfig.app.json", | ||
}, | ||
}, |
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.
outdated format
"dev": "export NODE_OPTIONS=--openssl-legacy-provider && VUE_CLI_SERVICE_CONFIG_PATH=$PWD/vue.config.app.js ./node_modules/.bin/vue-cli-service build --mode development --watch && tsc -p ./tsconfig.app.json && VUE_CLI_SERVICE_CONFIG_PATH=$PWD/vue.config.library.js ./node_modules/.bin/vue-cli-service build --mode development --watch", | ||
"dev": "VUE_CLI_SERVICE_CONFIG_PATH=$PWD/vue.config.app.js ./node_modules/.bin/vue-cli-service build --mode development --watch && tsc -p ./tsconfig.app.json && VUE_CLI_SERVICE_CONFIG_PATH=$PWD/vue.config.library.js ./node_modules/.bin/vue-cli-service build --mode development --watch", |
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.
export NODE_OPTIONS=--openssl-legacy-provider
with the update this flag isn't necessary anymore
@@ -1,4 +1,4 @@ | |||
import { shallowMount } from "@vue/test-utils"; | |||
import { shallowMount, VueWrapper } from "@vue/test-utils"; |
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.
IIRC correctly, these changes (both in HomeCardList and HomeView) were merged to main with another PR, but somehow when I applied the rebase they ended up becoming a separate commit.
value === 'true' || | ||
value === true || | ||
(prop.defaultValue === 'true' && | ||
(value === 'false' || value === false)) | ||
innerValue === 'true' || | ||
(prop.defaultValue === 'true' && innerValue === 'false') |
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.
Fixed vue issues. I didn't add tests to limit the scope of the PR.
}; | ||
config.output.library = "rundeckUiTrellis"; |
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.
Removed as libraryTarget with value of commonjs2 doesn't work with library
property
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.
LGTM, did a spot check on a variety of the pages that might have been affected, but they all look good
from prop mutation up to invalid v-model - the only hint that something was wrong was the message that vue was failing to compile the template
9d19566
to
b625369
Compare
Is this a bugfix, or an enhancement? Please describe.
List of enhancement/maintanance:
Describe the solution you've implemented
Followed the guide https://webpack.js.org/migrate/5/ and the following order for updating:
updated webpack > other npm packages > errors that would prevent the project from running/building locally > tests /imports/syntax errors
Checks performed:
How to test:
core
settings (can also doenterprise
as well, but then please remember to run installUiTrellis first);Or test with a docker image;
Describe alternatives you've considered
In the past there have been efforts to move the FE to vite, but as it requires a bigger effort to move out from vue-cli+webpack to vite, updating webpack to the newest version was a compromise for improving build speed/security and also ensuring compatibility with new npm packages that require webpack 5.
Additional context
Added comments explaining what was done in this PR.
Also, it might be worth to consider to use other modes of devtools as it greatly improves build time (One run, which had other modes enabled had a performance gain of 40% https://app.circleci.com/pipelines/github/rundeck/rundeck/14735)
Final performance improvement on circle ci: 5%