-
Notifications
You must be signed in to change notification settings - Fork 99
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Migrate to Webpack v5 #2685
Migrate to Webpack v5 #2685
Conversation
Signed-off-by: Rudy Flores <68666202+rudyflores@users.noreply.github.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #2685 +/- ##
=======================================
Coverage 93.34% 93.34%
=======================================
Files 109 109
Lines 10148 10148
Branches 2134 2134
=======================================
Hits 9473 9473
Misses 674 674
Partials 1 1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Rudy Flores <68666202+rudyflores@users.noreply.github.com>
Signed-off-by: Rudy Flores <68666202+rudyflores@users.noreply.github.com>
Signed-off-by: Rudy Flores <68666202+rudyflores@users.noreply.github.com>
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.
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.
remembering to reinstall pnpm after each node update will have to be something I remember to do now lol don't even remember updating node since last time testing next. Thanks @t1m0thyj
LGTM! thanks for the updates @rudyflores
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.
Thanks for all your work on this @rudyflores! I do have a small request:
I ran into an issue that isn't directly related to this PR, but: I am unable to run the watch script after downgrading my Node version to v18.
On line 24 in .vscode/tasks.json
, the type is set to "process" instead of "shell" - because of this, the task fails on Windows with OS error 193 because it tries to execute the Linux/macOS binary (which is not a valid Win32 application).
Can you please update this type to "shell"? This should resolve the watch script failures on Windows.
Signed-off-by: Rudy Flores <68666202+rudyflores@users.noreply.github.com>
@traeok good catch! Should be all fixed now :) |
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, thanks for the tech. currency and for quickly resolving the watch
task 😁
I noticed after approving that the changelog headers have the update (1/30): I see that the changelog is still failing, not sure why that is though because the FTP extension changelog seems to have the right layout. |
Signed-off-by: Rudy Flores <68666202+rudyflores@users.noreply.github.com>
|
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! thanks @rudyflores for this update
Proposed changes
Fixes #2214
Webpack v5 migration docs: https://webpack.js.org/migrate/5/
Migrated our outdated webpack v4 to the newest release of webpack v5, this has introduced some really good things for our repo, as well as things that we should all know about when reviewing this PR:
Code Splitting and Minimization
Code splitting using vendor strategy was used for optimizing load times on our project, see here for reference on how that was achieved: https://blog.jakoblind.no/code-split-vendors-with-webpack-for-faster-load-speed/
Minimization of our code was also integrated into our bundle for Zowe Explorer, which now greatly reduces the
extension.js
file size using Webpack's built in minimzer using Terser and ESBuild: https://webpack.js.org/plugins/terser-webpack-plugin/Bundling for VS Code for web and VS Code Desktop Versions
VS Code has a recommended way of setting up initially the
webpack.config.js
, this repo now follows (mostly) those recommendations. A thing to note is that our repo currently usesfs
which are node.js API's that can't be used in the web and cause issues with bundling. We didn't notice this before with v4 since we were already bundling asnode
instead of VS Code's recommendedwebworker
target to support both platforms in one bundle. See more on this here: https://code.visualstudio.com/api/working-with-extensions/bundling-extensionFuture things to address
Webpack no longer resolves polyfills automatically like in v4, which means if we switch the target to
webworker
we need to add those polyfill resolutions through fallbacks: https://webpack.js.org/configuration/resolve/#resolvefallbackEven if the work above is done, we need to discuss the approach to handle
fs
being used across Zowe Explorer and Zowe Explorer API in order to bundle for web and support that too (if we choose to support it).Release Notes
Milestone: v3 pre-releases
Changelog: Migrated to Webpack v5
Types of changes
What types of changes does your code introduce to Zowe Explorer?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the revieweryarn workspace vscode-extension-for-zowe vscode:prepublish
has been executedFurther comments