Skip to content

Add preflight check to guard against wrong versions of webpack/eslint/jest higher up the tree - #3771

Merged
gaearon merged 6 commits into
react:nextfrom
gaearon:verify-pkg
Jan 13, 2018
Merged

Add preflight check to guard against wrong versions of webpack/eslint/jest higher up the tree#3771
gaearon merged 6 commits into
react:nextfrom
gaearon:verify-pkg

Conversation

@gaearon

Copy link
Copy Markdown
Contributor

It looks like this:

screen shot 2018-01-13 at 02 43 55

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):

SKIP_PREFLIGHT_CHECK=true

in .env file.

"Fixes" #1795.

@gaearongaearon changed the title Add preflight check to guard against webpack/eslint/jest higher up the treeAdd preflight check to guard against wrong versions of webpack/eslint/jest higher up the treeJan 13, 2018
@gaearongaearon added this to the 2.0.0 milestone Jan 13, 2018
@gaearon
gaearon requested a review from TimerJanuary 13, 2018 02:46

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@TimerTimerJan 13, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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}})

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We want to support YW so I think we shouldn’t stop.

What fails?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What fails?

react-scripts when used with Yarn Workspaces, not this PR (what #3435 is attempting to solve).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@TimerTimerJan 13, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we mention valid cases?

When not using YW or Lerna+Hoist, it's valid to nest like so:

/node_modules
/eslint@3.x
/client
/node_modules
/eslint@4.x
/react-scripts@2.x
/eslint@4.x (hoisted)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Open to other wordings!

@gaearon
gaearon merged commit 197028c into react:nextJan 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
@gaearon
gaearon deleted the verify-pkg branch January 13, 2018 20:23
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 (react#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
@gaearongaearon mentioned this pull request Jan 16, 2018
akstuhl pushed a commit to akstuhl/create-react-app that referenced this pull request Mar 15, 2018
…/jest higher up the tree (react#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 (react#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
@locklockBot locked and limited conversation to collaborators Jan 20, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@gaearon@Timer@facebook-github-bot