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

feat: add require stack & surface Yarn PnP #9681

Merged
merged 28 commits into from
Mar 25, 2020
Merged

Conversation

MarcoScabbiolo
Copy link
Contributor

Summary

  • Add a require stack printed relative to <rootDir> for convenience and linking when a MODULE_NOT_FOUND error is thrown by the resolver.
  • Throw errors thrown by Yarn PnP as-is. Jest was swallowing them and failing later attempting to resolve the module.

Solves #9594

Test plan

E2E tests included in pnp and resolve

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Sorry about the slow response, I've been swamped with dayjob stuff lately.

Also, please add a test for pnp

packages/jest-runtime/src/helpers.ts Outdated Show resolved Hide resolved
packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

this is great!

packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
packages/jest-runtime/src/RuntimeModuleNotFoundError.ts Outdated Show resolved Hide resolved
packages/jest-runtime/src/RuntimeModuleNotFoundError.ts Outdated Show resolved Hide resolved
e2e/__tests__/resolveNoFileExtensions.test.ts Outdated Show resolved Hide resolved
@MarcoScabbiolo
Copy link
Contributor Author

It should be all good now.

I've fixed another bug, when there is some kind of nesting of required modules (index.js > module1.js > module2.js). The "However Jest was able to find moduleName" error wiill always show up since the error will bubble up from module2 to module1 and when it looks for a sibling module with similar file extension, it will find the module itself (module1.js) and show the hint for a file that actually exists and is there.

I've fixed it by adding a moduleName property on ModuleNotFoundError to hold a reference to the actual module that could not be found, so when the runtime attempts to find a sibling file with a different extension looks only for a file that was not found. The test should throw module not found error if the module has dependencies that cannot be found at e2e/resolve/__tests__/resolve.test.js covers this scenario.

There should not be any breaking changes AFAIK. In case a ModuleNotFoundError is thrown by a resolver that is not the default one the runtime will still be able tu duck type it, and use the previous behaviour in case the resolver did not specify the module that failed to require (moduleName property).

@codecov-io
Copy link

Codecov Report

Merging #9681 into master will decrease coverage by 0.16%.
The diff coverage is 26.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9681      +/-   ##
==========================================
- Coverage   65.09%   64.92%   -0.17%     
==========================================
  Files         287      288       +1     
  Lines       12144    12184      +40     
  Branches     3007     3020      +13     
==========================================
+ Hits         7905     7911       +6     
- Misses       3604     3635      +31     
- Partials      635      638       +3     
Impacted Files Coverage Δ
packages/jest-resolve/src/ModuleNotFoundError.ts 0.00% <0.00%> (ø)
packages/jest-resolve/src/index.ts 44.72% <10.00%> (-3.00%) ⬇️
packages/jest-runtime/src/index.ts 64.75% <45.83%> (-1.00%) ⬇️
e2e/pnp/undeclared-dependency/index.js 50.00% <50.00%> (ø)

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 4a59daa...5a805e5. Read the comment docs.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

This is great, thank you for being patient with me!

@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 11, 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.

4 participants