Upgrade to webpack 2 #2380
Upgrade to webpack 2 #2380
Conversation
package.json
Outdated
| @@ -285,15 +285,15 @@ | |||
| "semver": "^5.3.0", | |||
| "shelljs": "^0.7.7", | |||
| "sinon": "^2.2.0", | |||
| "sri-stats-webpack-plugin": "^0.7.3", | |||
| "sri-stats-webpack-plugin": "github:sopherio/sri-stats-webpack-plugin#f23de19e1d84dab50e59076e05bcafa508ba64c4", | |||
There was a problem hiding this comment.
This is pretty unfortunate since the patch to make it webpack 2 compatible is just a dependency bump. More info: mikechau/sri-stats-webpack-plugin#71
There was a problem hiding this comment.
I wonder if we should consider an alternative lib e.g: https://github.com/waysact/webpack-subresource-integrity (there might be others, though this says it supports both webpack1 and 2).
The bit I'm not keen on is that this change relies on a fork, which in turn loads another fork. Whilst you've been able to specify a hash in the first case, you can't in the second. To fix potential integrity issue we'd need to have our own forks or, it might be easier to use something else?
| plugins: [ | ||
| new webpack.DefinePlugin({ | ||
| CLIENT_CONFIG: JSON.stringify(clientConfig), |
There was a problem hiding this comment.
In the shared getPlugins() array, the process.env.NODE_ENV value is added to DefinePlugin unlike before. This doesn't seem to cause any problems though.
webpack-common.js
Outdated
| // Replaces server config module with the subset clientConfig object. | ||
| new webpack.NormalModuleReplacementPlugin( | ||
| /config$/, 'core/client/config.js'), | ||
| // Substitutes client only config. |
There was a problem hiding this comment.
The comment should be updated to refer to logging.
webpack-common.js
Outdated
| test: /\.jpg$/, | ||
| use: [{ | ||
| loader: 'url-loader', | ||
| options: { limit: 10000, mimetype: 'image/jpeg' }, |
There was a problem hiding this comment.
10000 could be a const at the top of the file.
webpack.prod.config.babel.js
Outdated
| @@ -27,7 +24,9 @@ for (const app of appsBuildList) { | |||
| const settings = { | |||
| devtool: 'source-map', | |||
| context: path.resolve(__dirname), | |||
| progress: true, | |||
| devServer: { | |||
| progress: true, | |||
There was a problem hiding this comment.
What does this setting relate to - is it ok in a prod config?
There was a problem hiding this comment.
Yeah, it's just for the webpack dev server. I can just move it to the dev config though.
webpack-common.js
Outdated
| test: /\.svg$/, | ||
| use: [{ loader: 'svg-url-loader', options: { limit: 10000 } }], | ||
| }, { | ||
| test: /\.jpg$/, |
There was a problem hiding this comment.
Nice work on the DRY'ing up.
webpack-common.js
Outdated
| ...styleRules, | ||
| { | ||
| test: /\.svg$/, | ||
| use: [{ loader: 'svg-url-loader', options: { limit: 10000 } }], |
There was a problem hiding this comment.
Should this have a mimetype added?
There was a problem hiding this comment.
I just read the docs on these loaders. I can add one here but why are we specifying mime types at all? The docs say that a mime type is inferred from file extension.
There was a problem hiding this comment.
If it's not needed then that's less code to maintain here 👍
| options: { | ||
| context: path.resolve(__dirname), | ||
| postcss: [ | ||
| autoprefixer({ browsers: ['last 2 versions'] }), |
There was a problem hiding this comment.
One way it should be possible to know this is working is that if when it's disabled via conf the CSS should be smaller. That does assume there's something in the CSS that would need to be prefixed with though.
There was a problem hiding this comment.
Looks like it's working. I searched -ms-flex-pack in dist/amo-eb2fa8bf265a819a3276f84cff2ff9b2.css before and after to verify.
|
This is great - I made some comments, I think it might be nice to switch out the sri plugin to something better maintained. I also added @tofumatt to the reviewers since more eyes on this the merrier. This is probably one to land after the tag just so there's more time to fix up any nits that come up in use. |
|
@muffinresearch I was able to switch us to |
|
I had to do some fix-ups so I guess you'll have to dig for it in the diff: https://github.com/mozilla/addons-frontend/pull/2380/files#diff-c325001c4cdd93809a99cbe1cb1b201cR10 |
Fixes mozilla/addons#10112
Webpack 1 to 2 migration guide: https://webpack.js.org/guides/migrating/
Here's what I tested:
npm run dev:amo,npm run unittest:dev,bin/extract-localesI had trouble getting
npm run startto work to mimic production. Doingnpm run buildis fine and the server starts but the bundle URLs are a 404. How do you simulate the CDN locally? I see the bundle files indistbut I'm not sure how to serve them. Is there a special value to use foramoCDNandstaticHostin the config?I also had trouble figuring out how to test that the
postcss-loaderis working. Any ideas?