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

fix(jest-cli): use import-local to support global Jest installations #5304

Merged
merged 2 commits into from Jan 14, 2018

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Jan 14, 2018

Summary
We have some custom code to check for <rootDir>/node_modules/jest-cli and if it's there, use it. This is (from what I can understand) to support using a global Jest installation to run a local installation.

The problem is that it breaks the assumption of node resolution algorithms, leading to weird errors like #5119.

This PR replaces the logic within jest-cli to use import-local in the entry point, similar to how xo, webpack-dev-server etc. do it.

The theory is that you only want this logic when running jest as a binary, not when using the programmatic API. Might be considered a breaking change, dunno?

Fixes #5294.

Test plan
I'm not sure how to properly test installing jest globally. Is it good enough to do yarn link and running the global bin? If so, this works.

@codecov-io
Copy link

codecov-io commented Jan 14, 2018

Codecov Report

Merging #5304 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5304   +/-   ##
=======================================
  Coverage   61.25%   61.25%           
=======================================
  Files         205      205           
  Lines        6893     6893           
  Branches        4        4           
=======================================
  Hits         4222     4222           
  Misses       2670     2670           
  Partials        1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09e47d6...176b834. Read the comment docs.

@cpojer
Copy link
Member

cpojer commented Jan 14, 2018

Oh this is cool. Just to clarify we need the added dependency, does Resolver.findNodeModule not work for this use case?

@SimenB
Copy link
Member Author

SimenB commented Jan 14, 2018

Resolver.findNodeModule is for locating a module, which is not what we're doing here - we want to see if the file we are in right now (which may or may not be a global module) has en equivalent in the local node_modules and execute that instead. So it's not as much "where is this module?" as it's "do I already exist in the locally installed project?".

I'm sure we could add some way to have the same logic in jest-resolve, but it's not something jest would really benefit from - this is strictly for a binary's entry point and not something to use after that.

@cpojer cpojer merged commit 6e788b1 into jestjs:master Jan 14, 2018
@cpojer
Copy link
Member

cpojer commented Jan 14, 2018

Works for me. Thanks for the explanation.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to 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.

Jest doesn’t respect Node resolution when looking for the CLI
4 participants