-
Notifications
You must be signed in to change notification settings - Fork 213
Conversation
Updated description with current list of breaking changes. Will update with features and bugs soonish. |
@helfi92 feel free to start reviewing this whenever. I will add docs in a different PR. cc: @tj @TheLarkInn @ev1stensberg for any comments or opinions they may have. :) |
@@ -0,0 +1,5 @@ | |||
const { EnvironmentPlugin } = require('webpack'); | |||
|
|||
module.exports = ({ config }, envs = []) => config |
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 second argument automatically scoped to the middleware from the "options" in package.json? Or the whole object? Or neither hahah
EDIT: nvm I see, presets are slightly different than middleware accessing neutrino.options
instead of passing that object.
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.
At the moment, no, it's just whatever gets passed to the middleware.
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.
Although it would be cool to map a name to preset/middleware options automatically, but that would probably require passing a name along with the preset/middleware. Which is possible.
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.
Might be cool to unify middleware/presets in some way like that, not a big deal though
awesome LGTM at a quick glance! Thanks a ton for working on this, amazing how much work it removes |
"bugs": "https://github.com/mozilla-neutrino/neutrino-dev/issues", | ||
"dependencies": { | ||
"progress-bar-webpack-plugin": "^1.9.3", | ||
"webpack": "^2.2.1" |
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.
Does this middleware need webpack as a dep??
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.
Nope, that's a mistake lol.
"description": "Neutrino preset for testing Neutrino projects with Karma", | ||
"main": "src/index.js", | ||
"main": "index.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.
Is there a reason you took index.js
outside src
?
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.
Just to flatten the directory structure for packages that don't need the extra complexity.
packages/neutrino/bin/neutrino
Outdated
|
||
try { | ||
pkg = require(join(cwd, 'package.json')); | ||
} catch (ex) {} |
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 we throw an exception message?
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.
No, we try to load data from package.json if it exists, but it's not going to affect anything if we can't find it.
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.
Ah ok, makes sense.
@eliperelman Want to exhange emails instead? I need some insight on the entire thing from multiple sources, so I can elaborate better :) |
@ev1stensberg sure, eli@mozilla.com. |
If there are no major objections, I plan on merging this later today. |
I made some changes to webpack-chain, now v3, which makes interacting with some APIs different, but more extensible. The Neutrino API now also defines defaults for options which mostly eliminate the need for presets to do their own path lookups: neutrino.options : {
root, // defaults to process.cwd()
source, // defaults to process.cwd()/src
output, // defaults to process.cwd()/build
tests, // defaults to process.cwd()/test
entry, // defaults to process.cwd()/src/index.js
node_modules // defaults to process.cwd()/node_modules
}; All the presets now use these at This also means that you can easily override how a single option affects a number of configuration changes: {
"neutrino": {
"options": {
"source": "lib",
"entry": "entry.js"
}
}
} const api = new Neutrino({ source: 'lib', entry: 'entry.js' }); module.exports = neutrino => {
neutrino.options.source = 'lib';
neutrino.options.entry = 'entry.js';
} For example, the Web preset needs the source directory (not to mention other directories) for:
w00t! |
Just to contrast, let's say you wanted to change the web/react presets to use module.exports = ({ config }) => {
const oldSrc = path.join(__dirname, 'src');
const newSrc = path.join(__dirname, 'lib');
const oldEntry = path.join(oldSrc, 'index.js');
const newEntry = path.join(newSrc, 'index.js');
config
.entry('index')
.delete(oldEntry)
.add(newEntry);
config.module
.rule('compile')
._include.delete(oldSrc);
config.module.rule('compile').add(newSrc)
config.devServer.contentBase(newSrc);
if (process.env.NODE_ENV !== 'development') {
config
.plugin('copy')
.inject((Plugin, args) => {
args[0].context = newSrc;
return new Plugin(...args);
});
}
}; eww. Now v5: {
"neutrino": {
"options": {
"source": "lib"
}
}
} Or from override file: neutrino.options.source = 'lib'; Ahh. |
const merge = require('deepmerge'); | ||
|
||
module.exports = ({ config }, options) => { | ||
const { paths, root } = merge({ paths: [], root: process.cwd() }, 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.
Need to switch this to neutrino.options.root
.
.travis.yml
Outdated
yarn: true | ||
directories: | ||
- node_modules | ||
- packages/neutrino-middleware-chunk/node_modules |
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 list also include these entries?
packages/neutrino-middleware-banner/node_modules
packages/neutrino/node_modules
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.
Doh, yes.
.travis.yml
Outdated
- packages/neutrino-preset-react/node_modules | ||
- packages/neutrino-preset-web/node_modules | ||
before_script: | ||
- yarn |
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.
Travis by default runs yarn install
as part of the install
step (if not overridden), so this additional yarn
invocation is a duplicate, eg:
https://travis-ci.org/mozilla-neutrino/neutrino-dev/jobs/209062877#L274
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! This is an artifact left over from when I switched from install
to before_script
in 573c821. I'll update. 👍
I will add README.md's to the middleware packages after this and #98 have been merged. |
🎉 |
Aside from #58, is there much else to do before v5 is published? I'm just trying to work out whether we should switch to v5 now for the Treeherder Neutrino PR, or wait until later. |
@edmorley right now nothing more is planning regarding functionality, and late last night I published beta packages of everything to npm, e.g. |
* Docs: Sync the READMEs in docs/ and packages/ Since the content had drifted out of sync. * Docs: Use consistent case for Hot Module Replacement * Docs: Sync the react and web preset feature lists * Docs: Correct the loader name for the 'html' rule Since the loader name was changed from `file` to `html` in #219. * Docs: Correct the rule name for neutrino-middleware-style-loader Since it was changed from `css` to `style` in #86.
Summary
BREAKING CHANGES
webpack-chain also has moved rule includes and excludes to
ChainedMap
s:neutrino
object.neutrino
object. No moreneutrino.custom
.neutrino
object.css
tostyle
, i.e.config.module.rule('style')
.testPathDirs
toroots
. Jest options through package.json are now done vianeutrino.options.jest
solely.neutrino-middleware-eslint
.neutrino.custom.eslintrc()
toneutrino.eslintrc()
.options
object to store at.options
.api.use()
or call it with the API and options, i.e.preset(neutrino, options)
.getWebpackOptions()
no longer caches calls.node
target no longer skips the watcher for a builder, it now uses the Webpack watcher, i.e.neutrino start && node build
is obsolete.neutrino build && node build
would work to start a Node instance for production-built bundles.Features
neutrino.options.root
, or for entry, relative toneutrino.options.source
:--inspect
flag to output a string representation of the Webpack configuration object. Can be used to diff configuration changes:packages/*/test
Bugs