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

RUN-2221: Update webpack 4 to 5 #8968

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Conversation

smartinellibenedetti
Copy link
Contributor

@smartinellibenedetti smartinellibenedetti commented Feb 29, 2024

Is this a bugfix, or an enhancement? Please describe.

List of enhancement/maintanance:

  • Update webpack 4 to 5;
  • Update packages related to vue-cli and jest;
  • Remove/fix/update imports that had issues - some were wrong, some aren't needed anymore;
  • Remove commands that were necessary only for webpack 4;
  • Small fixes (especially syntax) that would prevent builds or tests from running;

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:

  • ran final image with docker;
  • ran locally all commands used by the project;

How to test:

  • run this branch locally, firstly npm install then run project's core settings (can also do enterprise as well, but then please remember to run installUiTrellis first);
  • check for any missing files or errors in the UI;

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%

@smartinellibenedetti smartinellibenedetti changed the title Spike to update webpack 4 to 5 RUN-2211: Update webpack 4 to 5 Apr 29, 2024
@@ -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" />
Copy link
Contributor Author

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"/>
Copy link
Contributor Author

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" />
Copy link
Contributor Author

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.

Comment on lines -65 to -67
<!-- VUE CSS MODULES -->
<asset:stylesheet href="static/css/components/version-notification.css"/>
<!-- /VUE CSS MODULES -->
Copy link
Contributor Author

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"/>
Copy link
Contributor Author

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.

Copy link
Member

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

@@ -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"/>
Copy link
Contributor Author

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"/>
Copy link
Contributor Author

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)}"/>
Copy link
Contributor Author

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"/>
Copy link
Contributor Author

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
Copy link
Contributor Author

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:

https://webpack.js.org/configuration/devtool/#devtool

Comment on lines -3 to -7
globals: {
"ts-jest": {
tsconfig: "tsconfig.app.json",
},
},
Copy link
Contributor Author

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",
Copy link
Contributor Author

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";
Copy link
Contributor Author

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.

Comment on lines -6 to +7
value === 'true' ||
value === true ||
(prop.defaultValue === 'true' &&
(value === 'false' || value === false))
innerValue === 'true' ||
(prop.defaultValue === 'true' && innerValue === 'false')
Copy link
Contributor Author

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.

@smartinellibenedetti smartinellibenedetti added this to the 5.3.0 milestone May 1, 2024
@smartinellibenedetti smartinellibenedetti marked this pull request as ready for review May 1, 2024 03:46
@smartinellibenedetti smartinellibenedetti changed the title RUN-2211: Update webpack 4 to 5 RUN-2221: Update webpack 4 to 5 May 1, 2024
};
config.output.library = "rundeckUiTrellis";
Copy link
Contributor Author

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

Copy link
Member

@gschueler gschueler left a 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

@smartinellibenedetti smartinellibenedetti modified the milestones: 5.3.0, 5.4.0 May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants