-
Notifications
You must be signed in to change notification settings - Fork 213
Upgrading Webpack to v3, merge config as middleware #315
Conversation
Could you hold off releasing the new version until someone does a review pass? :-) |
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.
Some awesome changes here! Can't wait to try webpack 3 with Treeherder and see what improvement it makes to build times. A few of the other fixes here will also mean fewer workarounds/even less boilerplate in the soon to be landing treeherder .neutrinorc.js
:-)
| `compile` | Compiles JS files from the `src` directory using Babel. Contains a single loader named `babel`. From `neutrino-middleware-compile-loader`. | all | | ||
| `html` | Allows importing HTML files from modules. Contains a single loader named `html`. From `neutrino-middleware-html-loader`. | all | | ||
| `html` | Allows importing HTML files from modules. Contains a single loader named `file`. From `neutrino-middleware-html-loader`. | all | |
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 this is an accidental revert of part of my changes from #325 which occurred when master was merged into this PR. I'll open a PR to reapply those changes :-)
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.
This and the other docs parts fixed in #335
@@ -280,21 +281,22 @@ The following is a list of plugins and their identifiers which can be overridden | |||
|
|||
_Note: Some plugins are only available in certain environments. To override them, they should be modified conditionally._ | |||
|
|||
| Name | Description | Environments | | |||
| ---- | ----------- | ------------ | | |||
### Override configuration |
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.
Here too
- **BREAKING CHANGE** ESLint has been upgraded to v4, which may bring linting configuration | ||
changes. | ||
- **BREAKING CHANGE** The Neutrino `static` option has been removed. The Web, React, and Node.js presets will | ||
still copy files from a `src/static` directory if it exists, but you can override your own |
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.
Is the plan to eventually remove this copying from src/static
entirely? (The removal of the section mentioning it from project-layout.md
just made me think this might be the case?).
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 copying from src/static
is fine by default, but any overrides of copy would obviously negate that. What wasn't sustainable was providing an option for the static directory, since it just wasn't powerful enough or didn't operate as expected.
16 02 2017 10:36:34.715:INFO [launcher]: Launching browser Chrome with unlimited concurrency | ||
16 02 2017 10:36:34.731:INFO [launcher]: Starting browser Chrome | ||
16 02 2017 10:36:35.655:INFO [Chrome 56.0.2924 (Mac OS X 10.12.3)]: Connected on socket MkTbqJLpAAa2HFaeAAAA with id 21326158 | ||
16 02 2017 10:36:35.655:INFO [Chrome 60 (Mac OS X 10.12.3)]: Connected on socket MkTbqJLpAAa2HFaeAAAA with id 21326158 |
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.
These changes and a few others need syncing to packages/*
- I'll include them in the same PR as above :-)
@@ -12,14 +12,14 @@ | |||
- Automatic creation of HTML pages, no templating necessary | |||
- Hot Module Replacement support | |||
- Tree-shaking to create smaller bundles | |||
- Production-optimized bundles with Babili minification and easy chunking | |||
- Production-optimized bundles with Babili minification, easy chunking, and scope-hoisted modules for faster execution |
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.
This needs syncing to the docs/presets/*
copies, plus to the react preset "includes from web" list
@@ -9,7 +9,7 @@ install: | |||
- yarn install --frozen-lockfile | |||
before_install: | |||
# Required due to: https://github.com/travis-ci/travis-ci/issues/7951 | |||
- curl -sSfL https://yarnpkg.com/install.sh | bash | |||
- curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --version 0.25.4 |
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 see you've filed guigrpa/oao#50 for issues with oao on newer yarn. Perhaps it's worth adding a link to that here?
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.
Yeah, good idea.
The `babili` and `overrides` properties map to the options defined by | ||
[babili-webpack-plugin](https://www.npmjs.com/package/babili-webpack-plugin#options). | ||
The `minify` and `plugin` properties map to the options defined by | ||
[babel-minify-webpack-plugin](https://www.npmjs.com/package/babel-minify-webpack-plugin#options). |
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.
The #options
anchor tag doesn't appear to work on the NPM package page. Perhaps we should link to https://github.com/webpack-contrib/babel-minify-webpack-plugin#options instead?
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'm cool with doing 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.
Fixed in #340
You can pass any additional option Jest accepts. See the [Jest documentation](https://facebook.github.io/jest/docs/configuration.html#collectcoveragefrom-array) for more configuration options. | ||
You can pass any additional option Jest accepts. See the | ||
[Jest documentation](https://facebook.github.io/jest/docs/configuration.html#collectcoveragefrom-array) for more | ||
configuration options. |
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.
This needs syncing to packages/*
const options = merge.all([ | ||
opts, | ||
!opts.include && !opts.exclude ? { include: [neutrino.options.source], exclude: [neutrino.options.static] } : {} | ||
!opts.include && !opts.exclude ? { include: [neutrino.options.source] } : {} |
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.
Should this now be:
!opts.include ? { include: [neutrino.options.source] } : {}
This would allow people to specify an exclude list, but keep the default include, saving having to pass it unnecessarily (Treeherder hits this case for example).
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.
Yeah, good catch.
@@ -3,18 +3,17 @@ const merge = require('deepmerge'); | |||
module.exports = ({ config }, options = {}) => { | |||
const { limit } = merge({ limit: 8192 }, options); |
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'm not sure if this is expected (ie to prevent the presets being overwhelmed with options), but none of the presets actually pass in any options to this or some of the other middleware - so this functionality is only being used by those who call the middleware manually.
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.
Yeah, we don't use this internally, this is just for consumers of the middleware who happen to want to override options.
@@ -98,10 +95,6 @@ module.exports = (neutrino, opts = {}) => { | |||
.chunkFilename('[name].[chunkhash].js') | |||
.end() | |||
.resolve | |||
.alias | |||
// Make sure 2 versions of "core-js" always match in package.json and babel-polyfill/package.json | |||
.set('core-js', dirname(require.resolve('core-js'))) |
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 just spotted core-js
is now unused after this and can be removed from package.json
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.
Good catch.
@@ -1 +0,0 @@ | |||
import 'babel-polyfill'; |
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.
The babel-polyfill
package is now unused too and can be removed from package.json
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.
Definitely, good catch.
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've opened #339 to remove the unused deps
* Move preset-airbnb-base dependencies out of middleware-eslint The `eslint-config-airbnb-base` and `eslint-plugin-import` packages are not used by `neutrino-middleware-eslint` itself, but instead by `neutrino-preset-airbnb-base`. * Remove unused dependencies Of note: * `webpack-dev-middleware` can be removed from the karma preset, since it's only used by `karma-webpack` which already has it as a dependency. * `webpack-dev-server` can be removed from the web preset since it's a leftover from when `neutrino-middleware-dev-server` was created. * the web preset's `core-js` and `babel-polyfill` dependencies are leftover from #315. Everything else was manually determined to be unused via correlating the package list against greps of the source, plus checking that the package wasn't a peer dependency of another listed package. * Regenerate yarn lockfiles
See: #315 (comment) Also restores the best practices of using `-f` with curl, as well as quietening the log spam.
Previously if `exclude` was set manually, it would prevent the default value for `include` being used. Now consumers of the middleware can pass just `exclude` and not have to pass both. See: #315 (comment) The default include in neutrino-preset-airbnb-base has also been removed, since it just duplicated that in neutrino-middleware-eslint.
Since the option was removed in #315.
No description provided.