Skip to content
This repository has been archived by the owner on Feb 8, 2023. It is now read-only.

Webpack 2 #355

Closed
wants to merge 4 commits into from
Closed

Webpack 2 #355

wants to merge 4 commits into from

Conversation

JoeStanton
Copy link
Contributor

@JoeStanton JoeStanton commented Jan 31, 2017

Motivation

Webpack 2 has now been released, it contains a number of nice new features. One feature we're not yet taking advantage of is tree shaking.

Unfortunately, most modules we depend upon are currently using CommonJS module syntax, not ES6 syntax, this means we only save 2.2% on the bundle with tree shaking enabled, this should increase a lot in the future.

Test plan

  • A basic regression test. Do the JS and styles load correctly? Does the build work? (Yes)

Pre-merge checklist

  • Documentation
  • Unit tests
  • Reviews
    • Code review
    • Design review
  • Manual testing
    • Windows 7 IE 11
    • Windows 10 Edge
    • Mac OS El Capitan Safari
    • Mac OS El Capitan Chrome
    • Mac OS El Capitan Firefox
    • iOS 9 Safari
    • Android 6 Chrome
    • Listen to the site on a screen reader
    • Navigate the site using the keyboard only
  • Tester approved

@@ -36,8 +36,8 @@
"eslint-plugin-import": "^1.15.0",
"eslint-plugin-jsx-a11y": "^2.2.2",
"eslint-plugin-react": "^6.2.1",
"extract-text-webpack-plugin": "^1.0.1",
"file-loader": "^0.9.0",
"extract-text-webpack-plugin": "grrowl/extract-text-webpack-plugin#order-undefined-error",
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 fork allows ignoring the undefined order of our CSS modules. We should fix this at its root cause but it's not entirely clear how to do that at the moment: webpack-contrib/extract-text-webpack-plugin#166

Copy link
Contributor

Choose a reason for hiding this comment

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

The "Order in extracted chunk" errors have decreased, but not gone. There were 44 and there are now 12. They all flag up against the same file ./site/components/header/style.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.

The point is that they don't fail the build with this fork. I did try and correct the issue in header/style.css but didn't make much progress. Practically, it won't matter unless there are styles with exactly the same level of specificity being generated in a non-deterministic order.

Copy link
Contributor

@grahammendick grahammendick left a comment

Choose a reason for hiding this comment

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

Webpack 2 has actually increased the bundle size, compared with master (Webpack 1). It's about 6% bigger (9% without tree shaking).

@@ -36,8 +36,8 @@
"eslint-plugin-import": "^1.15.0",
"eslint-plugin-jsx-a11y": "^2.2.2",
"eslint-plugin-react": "^6.2.1",
"extract-text-webpack-plugin": "^1.0.1",
"file-loader": "^0.9.0",
"extract-text-webpack-plugin": "grrowl/extract-text-webpack-plugin#order-undefined-error",
Copy link
Contributor

Choose a reason for hiding this comment

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

The "Order in extracted chunk" errors have decreased, but not gone. There were 44 and there are now 12. They all flag up against the same file ./site/components/header/style.css

{ allChunks: true }
),
new webpack.LoaderOptionsPlugin({ options: { context: __dirname, postcss: [autoprefixer] } }),
new ExtractTextPlugin({ allChunks: true, filename: 'assets-honestly/styles-[contenthash:base64:5].css', ignoreOrder: true }),
Copy link
Contributor

Choose a reason for hiding this comment

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

This same ExtractTextPlugin change needs to go in webpack.dev.browser.config.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.

Ah yes - I won't be able to work on this today though.

@asavin
Copy link
Contributor

asavin commented Mar 2, 2017

Closing this one for now as it is stale. Feel free to re-open.

@asavin asavin closed this Mar 2, 2017
@dpiatek dpiatek deleted the webpack-2 branch December 5, 2018 11:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants