Add preflight check to guard against wrong versions of webpack/eslint/jest higher up the tree #3771
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gaearon
changed the title
Timer
approved these changes
Jan 13, 2018
| fs.readFileSync(maybeDepPackageJson, 'utf8') | ||
| ); | ||
| const expectedVersion = expectedVersionsByDep[dep]; | ||
| if (depPackageJson.version !== expectedVersion) { |
There was a problem hiding this comment.
Assuming the package is found and the version matches, do we need to continue looking up the tree?
In this case, we could splice it out of the array.
Our dependencies should never be hoisted out of our project [except in the case of Yarn Workspaces] and this fails spectacularly currently.
I'm fine with this if we want to be cautious and support Yarn Workspaces in the future
There was a problem hiding this comment.
It's a shame we have to look multiple levels, because if we only needed the first occurrence we could be sly and use require (simplifying this logic):
depsToCheck.map(name => {
try {
const { version } = require(`${name}/package.json`)
// ...
} catch (e) {
// ignored
}
})There was a problem hiding this comment.
We want to support YW so I think we shouldn’t stop.
What fails?
There was a problem hiding this comment.
Also just one level doesn’t help. In my example we’ll still find “our own” webpack but the project above might have its copy.
There was a problem hiding this comment.
What fails?
react-scripts when used with Yarn Workspaces, not this PR (what #3435 is attempting to solve).
There was a problem hiding this comment.
Also just one level doesn’t help. In my example we’ll still find “our own” webpack but the project above might have its copy.
You're right. This LGTM.
Timer
reviewed
Jan 13, 2018
| )} package (apart from the expected ${chalk.bold( | ||
| 'react-scripts' | ||
| )}) installed ${chalk.bold(dep)}.\n\n` + | ||
| `If nothing else helps, add ${chalk.bold( |
There was a problem hiding this comment.
Should we mention valid cases?
When not using YW or Lerna+Hoist, it's valid to nest like so:
/node_modules
/[email protected]
/client
/node_modules
/[email protected]
/[email protected]
/[email protected] (hoisted)
There was a problem hiding this comment.
Open to other wordings!
This ensures we don't try to filter NODE_PATH twice, accidentally removing the now-absolute path.
This was referenced Jan 13, 2018
gaearon
added a commit
that referenced
this pull request
Jan 13, 2018
…/jest higher up the tree (#3771) * Run real scripts in local development * Add preflight check warning * I know what I am doing * Move preflight check into individual scripts This ensures we don't try to filter NODE_PATH twice, accidentally removing the now-absolute path. * Slightly tweak the wording * Fix lint
Timer
pushed a commit
that referenced
this pull request
Jan 14, 2018
…/jest higher up the tree (#3771) * Run real scripts in local development * Add preflight check warning * I know what I am doing * Move preflight check into individual scripts This ensures we don't try to filter NODE_PATH twice, accidentally removing the now-absolute path. * Slightly tweak the wording * Fix lint
gaearon
added a commit
that referenced
this pull request
Jan 14, 2018
…/jest higher up the tree (#3771) * Run real scripts in local development * Add preflight check warning * I know what I am doing * Move preflight check into individual scripts This ensures we don't try to filter NODE_PATH twice, accidentally removing the now-absolute path. * Slightly tweak the wording * Fix lint
gaearon
added a commit
that referenced
this pull request
Jan 14, 2018
…/jest higher up the tree (#3771) * Run real scripts in local development * Add preflight check warning * I know what I am doing * Move preflight check into individual scripts This ensures we don't try to filter NODE_PATH twice, accidentally removing the now-absolute path. * Slightly tweak the wording * Fix lint
gaearon
added a commit
that referenced
this pull request
Jan 14, 2018
…/jest higher up the tree (#3771) * Run real scripts in local development * Add preflight check warning * I know what I am doing * Move preflight check into individual scripts This ensures we don't try to filter NODE_PATH twice, accidentally removing the now-absolute path. * Slightly tweak the wording * Fix lint
gaearon
added a commit
that referenced
this pull request
Jan 14, 2018
…/jest higher up the tree (#3771) * Run real scripts in local development * Add preflight check warning * I know what I am doing * Move preflight check into individual scripts This ensures we don't try to filter NODE_PATH twice, accidentally removing the now-absolute path. * Slightly tweak the wording * Fix lint
Timer
pushed a commit
to Timer/create-react-app
that referenced
this pull request
Jan 15, 2018
…/jest higher up the tree (facebook#3771) * Run real scripts in local development * Add preflight check warning * I know what I am doing * Move preflight check into individual scripts This ensures we don't try to filter NODE_PATH twice, accidentally removing the now-absolute path. * Slightly tweak the wording * Fix lint
akstuhl
pushed a commit
to akstuhl/create-react-app
that referenced
this pull request
Mar 15, 2018
…/jest higher up the tree (facebook#3771) * Run real scripts in local development * Add preflight check warning * I know what I am doing * Move preflight check into individual scripts This ensures we don't try to filter NODE_PATH twice, accidentally removing the now-absolute path. * Slightly tweak the wording * Fix lint
zmitry
pushed a commit
to zmitry/create-react-app
that referenced
this pull request
Sep 30, 2018
…/jest higher up the tree (facebook#3771) * Run real scripts in local development * Add preflight check warning * I know what I am doing * Move preflight check into individual scripts This ensures we don't try to filter NODE_PATH twice, accidentally removing the now-absolute path. * Slightly tweak the wording * Fix lint
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It looks like this:
I thought about extracting it to gist but I kinda like that we can substitute the dep name right into the message. We might also want to do something similar for duplicate React.
There is an opt-out mechanism mentioned at the very end (cc @jlongster):
in
.envfile."Fixes" #1795.