-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
chore(webpack): first pass on better webpack build #7038
base: master
Are you sure you want to change the base?
Conversation
@unlikelyzero test this thoroughly with both production and development builds. Have tested it with devServer but you need to make sure it doesnt break anything on your end. |
Current Playwright Test Results Summary✅ 162 Passing - Run may still be in progress, this comment will be updated as current testing workflow or job completes... (Last updated on 12/09/2023 03:33:10pm UTC)
|
|
2 Test Cases Affected |
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Restricted Notebook with a page locked and with an embed @addinit Allows embeds to be deleted if page unlocked @addinit
Retry 1 • Initial Attempt |
0% (0)0 / 39 runsfailed over last 7 days |
56.41% (22)22 / 39 runsflaked over last 7 days |
Restricted Notebook with a page locked and with an embed @addinit Disallows embeds to be deleted if page locked @addinit
Retry 1 • Initial Attempt |
2.56% (1)1 / 39 runfailed over last 7 days |
41.03% (16)16 / 39 runsflaked over last 7 days |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7038 +/- ##
==========================================
- Coverage 56.07% 55.98% -0.10%
==========================================
Files 654 655 +1
Lines 26228 26251 +23
Branches 2528 2528
==========================================
- Hits 14707 14696 -11
- Misses 10825 10852 +27
- Partials 696 703 +7
*This pull request uses carry forward flags. Click here to find out more.
... and 13 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
@evenstensberg hey this is great! Before we dive into the structure of it, can you spend one or two cycles figuring out why the tests are failing? |
@unlikelyzero ok now? PTAL |
@unlikelyzero what do you think of a dev setup with prod + dev builds that runs openmct locally (with unique bundles and chunks), and for customers, you'll have an unique build that only generates one bundle (dependency following your grammar) per entry? |
@evenstensberg If I run |
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.
Looking good so far! I've just left a couple questions for you.
BTW, I converted the original issue linked to this PR to a discussion as it seemed to fit better on the discussion board. Could you please create a new issue identifying the enhancements needed for our webpack config and then link it to this PR? Please then update the PR description with details about your implementation.
I'll have a look. I enabled progress bar (progressplugin) for dev, so you can see progress. For prod I didn't use it so you save a bit on ci logging $$$ |
Have reproduced this, and is because we don't generate a css file in dev anymore. I'll check if there's a workaround without using miniextractcss in dev builds. |
@ozyx Added a temp workaround. Suggestions to your build below (you need to discuss this with the team).
In one of my previous projects, we bundled all global stylesheets with gulp so that the webpack build step is somewhat isolated from generating your global stylesheets or dependencies. |
@evenstensberg Sorry for the delay in reviewing this, we've been busy wrapping up a release. I'm going to take a look at this today. Thanks for your patience! |
Hmm. I rather like the idea of keeping this modular. Let me discuss this with the team. |
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.
@ozyx want me to rebase?
That'd be great! |
@ozyx PTAL |
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.
Please remove any detection of the NODE_ENV variable
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.
Looking good. Still need to do some cursory testing-- just a couple of changes requested for now
ptal @ozyx |
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.
Please address the test failures
This comment was marked as outdated.
This comment was marked as outdated.
PTAL @ozyx |
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're getting closer. I left some comments regarding the minimization options that were chosen here. The goal is to get the bundle size as small as possible with as least aggressive minimization options as possible.
.webpack/webpack.prod.js
Outdated
warnings: false, | ||
parse: { | ||
html5_comments: false | ||
}, | ||
mangle: true, | ||
module: false, | ||
toplevel: false, | ||
nameCache: null, | ||
ie8: false, | ||
keep_classnames: false, | ||
keep_fnames: false, | ||
safari10: 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.
We need to be very careful here as we see more e2e test flakes and odd behavior with some of these options.
What's the least aggressive we can be with minimization optimization while also shrinking the bundle size as much as possible? In my experiments I found we should be able to shrink openmct.js
from ~35MB to ~5MB. If we can get close to that with mostly default settings, we should opt for that.
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 think that you'll need to manually test each of these settings, and enable/disable yourself to customize the fine-grained optimizations to your build.
@ozyx PTAL. left a comment on the remaining optimization strategies. |
Use the default options provided by terser
@ozyx I removed the options in the terser plugin. Just use defaults for now. PTAL |
}, | ||
{ | ||
loader: 'sass-loader', | ||
// resolve-url-loader needs all loaders below its decl to have sourcemaps enabled |
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.
nice
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 the updates. Will defer to @ozyx for remaining scope
#6964
Describe your changes:
This PR covers the initial work on a better configuration for the openMCT application. It's a restructural change to the existing configurations along with some improvements in the build itself.
All Submissions:
Author Checklist
Reviewer Checklist