-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Proposed Single Config Webpack 4 #1313
Conversation
clean up unused scripts
Deploy preview for netlify-cms-www ready! Built with commit 442233b |
Deploy preview for cms-demo ready! Built with commit 442233b |
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.
Looks great so far, just a few questions. Thanks for all your work on this!
@@ -5,12 +5,11 @@ | |||
"main": "dist/cms.js", | |||
"scripts": { | |||
"start": "npm run dev", | |||
"dev": "webpack-serve webpack.dev.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.
webpack.dev.js
can be deleted now, right?
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.
Yes, thanks for heads up. Missed it.
"test": "jest --coverage", | ||
"test:watch": "jest --watch", | ||
"build": "cross-env NODE_ENV=production webpack-cli --config webpack.prod.js --display-error-details", | ||
"build:scripts": "cross-env NODE_ENV=production webpack-cli --config webpack.cli.js", | ||
"build": "cross-env NODE_ENV=production webpack-cli --display-error-details --env.production", |
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 any problem with removing the NODE_ENV
value, since we are not using it in the script? The mode
sets it in the actual bundle.
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 problem here is the bundle size without it, I am wondering if the mode is not setting it on windows? needs research.
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.
Looks like it's not getting passed through to Babel. Either way, we might as well leave it, not a big deal.
}, | ||
plugins, | ||
target: 'web', // Make web variables accessible to webpack, e.g. window | ||
devServer: { |
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.
Sorry, I guess I don't understand why the server change was necessary? I'm definitely not opposed to switching it back now that the WriteFile plugin has been upgraded, I just want to understand why.
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 was getting a schema error on the settings using server
:
× 「wds」: Invalid configuration object. Webpack has been initialised using a configuration object that does not match the API schema.
- configuration has an unknown property 'server'. These properties are valid:
object { mode?, amd?, bail?, cache?, context?, dependencies?, devServer?, devtool?, entry?, externals?, loader?, module?, name?, node?, output?, optimization?, parallelism?, performance?, plugins?, profile?, recordsInputPath?, recordsOutputPath?, recordsPath?, resolve?, resolveLoader?, stats?, target?, watch?, watchOptions?arallelism?, performance?, plugins?, profile?, recordsInputPath?, recordsOutputPath?, recordsPath?, resolve?, resolveLoader?, stats?, target?, watarallelism?, performance?, plugins?, profile?, recordsInputPath?, recordsOutputPath?, recordsPath?, resolve?, r
arallelism?, performance?, plugins?, profile?, recordsInputPath?, recordsOutputPath?, recordsPath?, resolve?,
arallelism?, performance?, plugins?, profile?, recordsInputPath?, recordsOutputPath?, recordsPath?, resolve?, resolveLoader?, stats?, target?, watch?, watchOptions? }
For typos: please correct them.
For loader options: webpack >= v2.0.0 no longer allows custom properties in configuration.
Loaders should be updated to allow passing options via loader options in module.rules.
Until loaders are updated one can use the LoaderOptionsPlugin to pass these options to the loader:
plugins: [
new webpack.LoaderOptionsPlugin({
// test: /\.xxx$/, // may apply this only for some modules
options: {
server: …
}
})
]
error An unexpected error occurred: "Command failed.
Exit code: 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.
It should be serve
, not server
. Anyway, it doesn't support function-based configs yet, so lets just go with dev-server
for now.
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.
Due to some special preprocessing that is part of webpack-cli, and not webpack proper, webpack-serve cannot fully support configs that export a function without creating a bit of a mess. We're working on a solution for this, but for the moment we don't recommend using webpack-serve with configs that export a function()
Right, this is the official quote. We can upgrade once they have it stable
}, | ||
context: path.join(__dirname, 'src'), | ||
module: { | ||
noParse: /localforage\.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.
This should be fine for production if I understand noParse
correctly, right?
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, lodash
could be included here also, but for lodash
maybe we should consider converting to lodash-es
instead, since it is supported by the lodash
package managers. Just a thought for later.
@talves Awesome, feel free to merge into the main branch whenever you are ready. |
- Summary
--env.[production, development]
settings- Test plan
Same tests as #1214
- A picture of a cute animal (not mandatory but encouraged)
〰