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: set MODULE_NOT_FOUND code when module is not resolved from paths #8487

Merged
merged 13 commits into from
Nov 9, 2019

Conversation

clarkdo
Copy link
Contributor

@clarkdo clarkdo commented May 23, 2019

Summary

Relate to nuxt/nuxt#5796

When we use require.resolve(module, { paths: [...] }), the code of thrown exception is not MODULE_NOT_FOUND which is inconsistent with Node.js require.resolve behaviour.

Test plan

@codecov-io
Copy link

Codecov Report

Merging #8487 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8487      +/-   ##
==========================================
- Coverage   62.25%   62.24%   -0.02%     
==========================================
  Files         266      266              
  Lines       10746    10748       +2     
  Branches     2625     2626       +1     
==========================================
  Hits         6690     6690              
- Misses       3471     3473       +2     
  Partials      585      585
Impacted Files Coverage Δ
packages/jest-runtime/src/index.ts 68.68% <0%> (-0.37%) ⬇️

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 c24934c...ee3f826. Read the comment docs.

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Thanks @clarkdo, I agree the require implementation should emulate Node's as much as possible. According to Node's docs on this (plus I tried it in the REPL), this is not specific to require.resolve, but behaves the same with regular require. It would be great if we could also make Jest's require work that way. I think there's multiple places creating "module not found" errors, but maybe we could just create a helper function for that?
Also, since this is a documented feature in the regular require, I think we'll need a test for this :)

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.

Missing a test as mentioned, and the changelog needs an update 🙂

e2e/resolve-with-paths/__tests__/resolveWithPaths.test.js Outdated Show resolved Hide resolved
packages/jest-resolve/src/ModuleNotFoundError.ts Outdated Show resolved Hide resolved
@clarkdo
Copy link
Contributor Author

clarkdo commented May 24, 2019

Missing a test as mentioned, and the changelog needs an update 🙂

😸 Thanks for the review, changes are applied.

I'll add test and changelog shortly

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.

CI is failing on node 6, otherwise this LGTM. Thanks!

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Node 6 failure looks super annoying, the lower error (stack trace looks different) probably causes the upper one (regex mismatch). Dropping the error subclass would probably be an easy although ugly fix. Or waiting until Node 6 is dropped 😄

@thymikee
Copy link
Collaborator

snapshots need to be updated once again 🙈

* LICENSE file in the root directory of this source tree.
*/

export default class ModuleNotFoundError extends Error {
Copy link
Collaborator

@thymikee thymikee May 28, 2019

Choose a reason for hiding this comment

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

The Node 6 snapshot failures show pretty important change (not sure why it's different on higher versions though), since now we don't point to the place error happened, but to the error itself. We have a utility Error class called ErrorWithStack – maybe you could play with it and extend it directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like jest-resolve doesn't depend jest-utils, so can I use Error.captureStackTrace instead of extend ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I can add dependency for jest-resolve, maybe we can move ModuleNotFoundError to jest-util as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure about that, @SimenB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/cc @SimenB

@thymikee thymikee requested a review from SimenB June 14, 2019 08:34
@SimenB
Copy link
Member

SimenB commented Nov 9, 2019

We've dropped node 6. I merged in master, hopefully this passes CI and we can finally merge this 👍

@SimenB SimenB merged commit 17f6fb7 into jestjs:master Nov 9, 2019
@clarkdo clarkdo deleted the fix/reesolve-paths-err-code branch November 9, 2019 22:16
@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.

6 participants