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

upgrade webpack to ^2.6.0 #2278

Merged
merged 3 commits into from
Jul 13, 2017
Merged

Conversation

stvnjacobs
Copy link
Member

@stvnjacobs stvnjacobs commented Jul 12, 2017

This brings manager and docs up to the latest version of webpack 2.6
release. It will allow for future improvements that are only available
in webpack 2+.

Changes include:

  • Updated syntax for webpack loaders. link
  • Use the babel-plugin-istanbul to instrument code for tests,
    as per recommendation by the webpack-contrib team. link
  • Use cross-env for all npm scripts to improve compatibility.

This addresses #2211

This brings manager and docs up to the latest version of webpack 2.6
release. It will allow for future improvements that are only available
in webpack 2+.

Changes include:

- Updated syntax for webpack loaders. [link](https://webpack.js.org/guides/migrating/#module-loaders-is-now-module-rules)
- Use the babel-plugin-istanbul to instrument code for tests,
  as per recommendation by the webpack-contrib team. [link](webpack-contrib/istanbul-instrumenter-loader#44 (comment))
- Use `cross-env` for all npm scripts to improve compatibility.
@eatonphil
Copy link
Contributor

What is cross-env?

@@ -26,7 +28,6 @@ _.plugins = [
warnings: false
}
}),
new webpack.optimize.DedupePlugin(),
new webpack.optimize.AggressiveMergingPlugin()
Copy link
Contributor

Choose a reason for hiding this comment

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

Numbers on new bundle size plz?

Copy link
Member Author

@stvnjacobs stvnjacobs Jul 12, 2017

Choose a reason for hiding this comment

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

manager size (Bytes)
this branch with webpack 2 1,979,664
current develop branch 1,900,556
+79,108
docs size (Bytes)
this branch with webpack 2 1,593,314
current develop branch 1,524,985
+68,329

The reason for removing it is here:
https://webpack.js.org/guides/migrating/#dedupeplugin-has-been-removed

I'll get some more references for changes in webpack2 that can be used to decrease the bundle size.

Copy link
Member Author

Choose a reason for hiding this comment

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

Linking to some tests that I ran. It includes a test using the recommended babel config for webpack2. It ends up being slightly smaller/equal to the current bundle after the change.
https://gist.github.com/stvnjacobs/9daa815d3a2cc179eb2604f93066a76b

@eatonphil
Copy link
Contributor

I know this is a task and all :p but could you (or @mparke) include a sentence or two on why this is useful/needed?

@eatonphil
Copy link
Contributor

@stvnjacobs yeah I'm not asking why you added it. I'm curious what it does

@eatonphil
Copy link
Contributor

Is any of this going to conflict with our plans to get into newer npm versions? I.e. is it better they be done together or not so much?

@coveralls
Copy link

Coverage Status

Coverage decreased (-7.3%) to 70.801% when pulling 117b05e on stvnjacobs:build/webpack-2 into d235b10 on linode:develop.

@stvnjacobs
Copy link
Member Author

@eatonphil in regards to cross-env, from their docs they describe it as a solution to:

Most Windows command prompts will choke when you set environment variables with NODE_ENV=production like that. (The exception is Bash on Windows, which uses native Bash.) Similarly, there's a difference in how windows and POSIX commands utilize environment variables. With POSIX, you use: $ENV_VAR and on windows you use %ENV_VAR%.

I didn't look under the hood, just followed what was used for build:webpack. The reason being tests also used environment variables. My understanding is it only affects/helps Windows users.

I know this is a task and all :p but could you (or @mparke) include a sentence or two on why this is useful/needed?

I'll try and write something up.

@mparke
Copy link
Contributor

mparke commented Jul 12, 2017

Is any of this going to conflict with our plans to get into newer npm versions? I.e. is it better they be done together or not so much?

This should not create any conflicts

I had previously looked to do them together, but should be do-able in chunks, getting to webpack-2 should allow us to resolve symlinks in npm5 for local package paths ( i.e. /components ) ( based on what i've read ), which is the primary goal of #2212

also see https://webpack.js.org/configuration/resolve/#resolve-symlinks and http://blog.npmjs.org/post/161081169345/v500

@mparke
Copy link
Contributor

mparke commented Jul 12, 2017

What is cross-env?

Same question, actually, and can we open a separate PR for it if possible?

@stvnjacobs
Copy link
Member Author

cross-env was removed from test scripts as it is not necessary nor relevant to upgrading webpack. I may open an issue for it, after doing a bit more research, if I think that it should be added.

@stvnjacobs
Copy link
Member Author

I know this is a task and all :p but could you (or @mparke) include a sentence or two on why this is useful/needed?

In addition to what @mparke has mentioned regarding symlinks, the main differentiator between Webpack 1 and 2 is that it now supports parsing ES2015 style module imports[1], rather than relying on Babel to convert them to CommonJS style. The benefit of this is that Webpack can more accurately parse the entire codebase, resulting in improvements to transformations and optimizations (one example being tree shaking[2][3]).

[1] https://auth0.com/blog/javascript-module-systems-showdown/
[2] https://webpack.js.org/guides/tree-shaking/
[3] https://medium.com/modus-create-front-end-development/webpack-2-tree-shaking-configuration-9f1de90f3233)

Note: Babel, in our current configuration, normalizes all imports to CommonJS style before passing on to Webpack. By adding "modules": false to the es2015 preset in .babelrc, we can begin to take advantage of these optimizations.

@eatonphil
Copy link
Contributor

Can you just set modules false here? I did that and didn't seem to be getting much of an improvement in size. But we might as well anyway.

Copy link
Contributor

@mparke mparke left a comment

Choose a reason for hiding this comment

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

tested manager and docs looks good

@mparke
Copy link
Contributor

mparke commented Jul 13, 2017

can we wait for any other changes until after merge @eatonphil

@eatonphil
Copy link
Contributor

lgtm for now. Let's get the size down though. Thanks @stvnjacobs

@eatonphil eatonphil merged commit 259c64f into linode:develop Jul 13, 2017
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.

5 participants