-
Notifications
You must be signed in to change notification settings - Fork 864
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
Build improvements #13
base: master
Are you sure you want to change the base?
Conversation
Thank you, that's a very interesting (and big) PR full of improvements. I had a look at your modifications and here are the few details I found.
Then I have some open questions:
I really like the way you reorganized the webpack config files, it makes the whole thing cleaner and easier. What you did with the package.web.json is a very good idea too. Impressive work. |
]) | ||
}); | ||
|
||
module.exports.module.loaders[0].loader = 'react-hot!babel-loader'; |
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 something like this is more readable and easy to understand because it reflects your idea of aggregating config (you add one loader to the previous ones at the beginning of the list)
loaders: baseConfig.module.loaders.unshift({
test: /\.js$/,
exclude: /node_modules/,
loader: 'react-hot'
})
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.
That wouldn't work because unshift returns the new array length, not the actual array. You'd have to use .concat for 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.
Oh yes sorry the version that I tested worked because I saved the result in the loaders
property instead of module.loaders
and the baseConfig object was still updated by the method.
As you mentionned a .concat
version of it would be fine like:
module: {
loaders: Array.prototype.concat([{
test: /\.js$/,
exclude: /node_modules/,
loader: 'react-hot'
}],
baseConfig.module.loaders)
}
Webpack has builtin functionality to do pretty much everything the Gruntfile does ; it makes no sense to use two different tools that do the same thing.
These commits rectify most of that situation.