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

Use default webpack module resolution #926

Merged
merged 1 commit into from Jun 4, 2018
Merged

Use default webpack module resolution #926

merged 1 commit into from Jun 4, 2018

Conversation

edmorley
Copy link
Member

@edmorley edmorley commented Jun 1, 2018

Since the custom module resolution is not required, and has been the cause of at least 3 bugs that I have spent hours debugging.

The vast majority of modules referenced by Neutrino already use require.resolve() and so are unaffected by this change. Those that do not (for example eslint presets, since eslint doesn't support specifying them as full paths), were not helped by this custom module resolution anyway, since the resolution happens outside of webpack, and so relied on hoisting even before this change.

The node_modules option has been removed in favour of using the Neutrino API (or else NODE_PATH). Customising module resolution is a massive footgun and should not be used in most cases.

Fixes #822.

Since the custom module resolution is not required, and has been the
cause of at least 3 bugs that I have spent hours debugging.

The vast majority of modules referenced by Neutrino already use
`require.resolve()` and so are unaffected by this change. Those that
do not (for example eslint presets, since eslint doesn't support
specifying them as full paths), were not helped by this custom
module resolution anyway, since the resolution happens outside of
webpack, and so relied on hoisting even before this change.

The `node_modules` option has been removed in favour of using the
Neutrino API (or else `NODE_PATH`). Customising module resolution
is a massive footgun and should not be used in most cases.

Fixes #822.
@edmorley edmorley added this to the v9 milestone Jun 1, 2018
@edmorley edmorley self-assigned this Jun 1, 2018
@eliperelman
Copy link
Member

@eliperelman eliperelman commented Jun 1, 2018

I think the major reason we used these node_modules settings was to test create-project with yarn link, but with verdaccio that's really not necessary now. 👍

@edmorley
Copy link
Member Author

@edmorley edmorley commented Jun 1, 2018

True :-)

Though I've tested this with a yarn linked Neutrino + presets into Treeherder, and other than the yarn lint part (which didn't work before either) everything else still worked for me (thanks to our thorough module.resolve() usage). So doesn't seem to be any downside :-)

@edmorley edmorley requested a review from eliperelman Jun 4, 2018
@@ -93,8 +93,6 @@ module.exports = neutrino => {
[extensionsToNames(style)]: require.resolve('./style-mock')
}),
bail: true,
// eslint-disable-next-line camelcase
coveragePathIgnorePatterns: [node_modules],
Copy link
Member

@eliperelman eliperelman Jun 4, 2018

Choose a reason for hiding this comment

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

We don't need this line any more?

Copy link
Member Author

@edmorley edmorley Jun 4, 2018

Choose a reason for hiding this comment

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

The default is the regexp pattern "/node_modules/" which matches all node_modules:
https://facebook.github.io/jest/docs/en/configuration.html#coveragepathignorepatterns-array-string

The previous value was the custom node_modules setting which no longer exists.

Copy link
Member

@eliperelman eliperelman Jun 4, 2018

Choose a reason for hiding this comment

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

OK perfect.

Copy link
Member

@eliperelman eliperelman left a comment

Love seeing all the red. 👍

@edmorley edmorley merged commit b6eb31f into neutrinojs:master Jun 4, 2018
2 checks passed
@edmorley edmorley deleted the simplify-module-resolution branch Jun 4, 2018
@edmorley edmorley mentioned this pull request Jun 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants