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: correctly resolve remapped directories #12373

Merged
merged 15 commits into from
Feb 17, 2022

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Feb 11, 2022

Summary

Old OP

For now, just a failing test.

The issue is that resolve tries to find a directory, and when there's a mapping, that doesn't exist. Not sure how, but resolve needs to find the closest package.json first, load it, pass it through filters, then try to request from disk. Or something like it - we need to map the request before resolve checks if it's on disk. Currently neither packageFilter nor pathFilter is called before resolves throws MODULE_NOT_FOUND.

https://github.com/browserify/resolve/blob/8641b137f5fc1c4d40164c3aae7f4e492c332e42/lib/sync.js#L207-L220

/cc @ljharb any ideas on best approach here?

Rewrites the logic that respects exports to

  1. check if it's a module that's being required (i.e. path does not start with . or is absolute)
  2. load its package.json from root using resolve so paths etc is respected
  3. if exports exist, resolve it and return the absolute path
  4. if any of the above checks bail, proceed with a "normal" resolve lookup, like we had before feat: support . in exports field #11919 and feat: support exports in package.json #11961

Fixes #12372

Test plan

Green CI (eventually)

@ljharb
Copy link
Contributor

ljharb commented Feb 11, 2022

I'm pretty unclear on the problem. When you say "remapped", do you mean like, jest config that's trying to map one folder path to another?

@SimenB
Copy link
Member Author

SimenB commented Feb 11, 2022

When exports maps from a non-existing directory to an existent one, like: https://github.com/facebook/jest/pull/12373/files#diff-19fd284b5181b406231ab6bd894afeeabe5afa1020dc19f1199f65154f58c038R15

(Unsure if link works, but look at diff in package.json of this PR)

@ljharb
Copy link
Contributor

ljharb commented Feb 11, 2022

ahh, right. yeah this is one of the bigger stumbling blocks i've ran into for resolve's implementation :-/ i don't have any easy solutions

@SimenB
Copy link
Member Author

SimenB commented Feb 11, 2022

Right, none of the hooks (except isDirectory) was called. I'll play with walking up file tree next week

@SimenB
Copy link
Member Author

SimenB commented Feb 12, 2022

From looking at https://nodejs.org/api/modules.html#all-together, and specifically LOAD_PACKAGE_EXPORTS, it seems we should only read the root package.json of the module (at least the others specify "closest" while this does not), meaning there should be no traversal like my original plan was. So I'll try to split up the import specifier before calling resolve and do the mapping then (if exports exists), before passing the already mapped specifier back to resolve. Might actually simplify the code somewhat

@SimenB
Copy link
Member Author

SimenB commented Feb 13, 2022

Have a somewhat working version of that now, but it fails when paths is supplied, which probably makes sense. So might have to do this inside resolve after all

@SimenB SimenB force-pushed the exports-other-directory branch 2 times, most recently from d193498 to c8c6981 Compare February 15, 2022 22:05
@SimenB
Copy link
Member Author

SimenB commented Feb 15, 2022

pushed up my half-working code, if by chance somebody comes over this and gives it a try before I get back to it myself 😀

@SimenB SimenB force-pushed the exports-other-directory branch from 5bec21e to 1385905 Compare February 17, 2022 12:47
// NPM3 flat dep tree)
const jestUtil = require('../../../packages/jest-util');
Copy link
Member Author

Choose a reason for hiding this comment

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

I honestly don't understand this test 🙈 Comes from #1572

Copy link
Member Author

Choose a reason for hiding this comment

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

comment above doesn't make much sense, you're not simulating a real environment by using a relative path to get into some module

@SimenB SimenB marked this pull request as ready for review February 17, 2022 14:00
@SimenB SimenB merged commit dec7683 into jestjs:main Feb 17, 2022
@SimenB SimenB deleted the exports-other-directory branch February 17, 2022 14:27
@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 Mar 20, 2022
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.

[Bug]: Cannot find module in jest v28
3 participants