Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

chore(infrastructure): update webpack v4.x #4847

Merged
merged 15 commits into from
Jun 28, 2019
Merged

Conversation

moog16
Copy link
Contributor

@moog16 moog16 commented Jun 19, 2019

fixes #4842

webpack3 (master branch):

  • running npm run dist:
    • 2:22
    • 2:30
    • 2:17
    • average: 143s

webpack4 (with 2 minified file build):
- running npm run dist:
- 1:09
- 1:09
- 1:12

webpack4 (with minified and non-minified builds):

  • running npm run dist:
    • 1:08
    • 1:08
    • 1:11
    • average: 71.7s

Build time decreased by 49.14%

@moog16 moog16 changed the title fix: update webpack:wq chore(infrastructure): update webpack v4.x Jun 19, 2019
Copy link
Contributor Author

@moog16 moog16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now the JS file and min.js file are both minified -- meaning they are exactly the same. This means we could remove the min.js file.

@codecov-io
Copy link

codecov-io commented Jun 20, 2019

Codecov Report

Merging #4847 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4847   +/-   ##
=======================================
  Coverage   98.86%   98.86%           
=======================================
  Files         119      119           
  Lines        5650     5650           
  Branches      749      749           
=======================================
  Hits         5586     5586           
  Misses         63       63           
  Partials        1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d551dfd...4bf3375. Read the comment docs.

@mdc-web-bot
Copy link
Collaborator

All 693 screenshot tests passed for commit d1a1ec9 vs. master! 💯🎉

@abhiomkar
Copy link
Collaborator

Why would be remove *.min.js files?

@abhiomkar
Copy link
Collaborator

abhiomkar commented Jun 24, 2019

Did you see any difference in build time compared to Webpack 3?

@moog16
Copy link
Contributor Author

moog16 commented Jun 24, 2019

by default they the bundles are all minified. So there's no need to have both as they are the same.

@abhiomkar
Copy link
Collaborator

We need to continue publishing both minified and un-minified versions like before. Un-minified version is useful for development / debugging.

@moog16
Copy link
Contributor Author

moog16 commented Jun 24, 2019

We can provide maps instead of un-minified (which we already actually do). I don't think un-minified is totally necessary.

@abhiomkar
Copy link
Collaborator

We shouldn't break existing publish structure as part of this upgrade, IMO.

I'm not expert in this area, but it is fairly common practice to publish minified & un-minified.

@moog16
Copy link
Contributor Author

moog16 commented Jun 24, 2019

Fair point! I'll make it exactly as it was.

@mdc-web-bot
Copy link
Collaborator

All 693 screenshot tests passed for commit 2981439 vs. master! 💯🎉

@moog16
Copy link
Contributor Author

moog16 commented Jun 24, 2019

@abhiomkar ready to go again

@mdc-web-bot
Copy link
Collaborator

All 693 screenshot tests passed for commit 5d50bbf vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 693 screenshot tests passed for commit 774b4ea vs. master! 💯🎉

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@mdc-web-bot
Copy link
Collaborator

All 699 screenshot tests passed for commit e9da478 vs. master! 💯🎉

main,
out: destFilename,
const main = path.join(PACKAGES_DIRECTORY, packageDirectory, './index.d.ts');
fs.access(main, fs.constants.F_OK, (err) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: error*

out: destFilename,
const main = path.join(PACKAGES_DIRECTORY, packageDirectory, './index.d.ts');
fs.access(main, fs.constants.F_OK, (err) => {
if (!err) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (error) {
  return;
}

Returns early if error exist. This avoids nested block and makes it easy to read.

@mdc-web-bot
Copy link
Collaborator

All 699 screenshot tests passed for commit 4bf3375 vs. master! 💯🎉

@moog16 moog16 added cla: yes and removed cla: no labels Jun 28, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@moog16 moog16 merged commit 1d1b42a into master Jun 28, 2019
@moog16 moog16 deleted the chore/update-webpack-4 branch June 28, 2019 18:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to Webpack 4
7 participants