Skip to content
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

Integrate Webpack Validator Into Webpack.Config.js #381

Closed
wants to merge 22 commits into from

Conversation

LukeSheard
Copy link
Contributor

@LukeSheard LukeSheard commented Aug 7, 2016

Closes #367 by warning the user of any errors that occur within the webpack config. In particular this is useful for anyone who integrates gatsby-node/modifyWebpackConfig into their workflow to watch for errors that occur from misuse of merging and configuring custom setups when it comes to build time.

The issue brought up by this is that many of the packages required warn about a Node Version less than 4.0.0. Travis shows that all the tests fail on the Node 0.12.x versions.

@KyleAMathews
Copy link
Contributor

I'd be ok bumping our minimum required node version to 4. Older versions are about to be unsupported in any case.

@LukeSheard
Copy link
Contributor Author

Sure. I guess this will mean a minor release to 0.13, so I've updated the error message to reflect that. You can change that though if you want.

@KyleAMathews
Copy link
Contributor

Code looks great! Will give more through review + make release in a couple days when done traveling.

Could you post an example of what an error message looks like?

/cc @kentcdodds

return module(config)
let exportedWebpackConfig = module(config)
if (!!modifyWebpackConfig) {
exportedWebpackConfig = modifyWebpackConfig(module(config), stage)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user does not return a webpack configurator object here the subsequent calls to exportedWebpackConfig.resolve() will throw a TypeError: Cannot read property 'resolve' of undefined

@benstepp
Copy link
Contributor

benstepp commented Aug 8, 2016

I think a couple of console.log's are left over.

What is going on with the notAbsolutePath?

Otherwise looks good.


Just a late night idea with poor naming. If this was in it's own file with an optional function argument that defaulted to the user's modification function, you could run unit tests with various config objects and modifyWebpackConfigs to ensure the correct errors are being returned given certain user changes to the config. I would stray away from testing the libraries in use, but rather check that the user never sees errors that are internal.

// modifyWebpackConfig try catch here
export default function ModifyAndValidateUserChangesToConfig(config, modifyWebpackConfig = modifyWebpackConfig) {
  // do all the things
  return config
}

@LukeSheard
Copy link
Contributor Author

@benstepp The notAbsolute path error is because webpack-validator doesn't allow for absolute paths and there's no option in the configuration to allow that from the documentation or source code. Since Gatsby relies on absolute paths I ignore those if there are any.

I'll modify the PR to use a ModifyAndValidateUserChangesToConfig for now though?

@LukeSheard
Copy link
Contributor Author

Is it further worth splitting the webpack.config internal functions into separate files as well for easier digestion? Eg:

webpack.config/
    output.js
    entry.js
    ...

@benstepp
Copy link
Contributor

I'd leave the rest of the webpack.config for now to keep the changes small, focused, and easy to understand.

I went ahead and checked out your changes and it seems the validator error occurs because gatsby adds a path to resolveLoader.root that doesn't exist on the filesystem. The problem exists here on line 433.

I feel like it would be better to extract resolveLoader into a function like everything else and do a fs.statSync for the folder before adding it. This way it's a little more clear about what is happening, we might gain some performance by reducing webpack scope, and we aren't suppressing errors that may benefit the user from webpack-validator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants