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: support . in exports field #11919

Merged
merged 8 commits into from
Oct 17, 2021
Merged

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Oct 1, 2021

Summary

If we don't have a main field, let's attempt to set it to the result of resolving . (i.e. "main" entry of package) if it exists. Since we do nothing unless main is already missing I think this is a safe thing to do and it's not a breaking change. Can revert and land for Jest 28 if it proves to be breaking, though.

Ref #9771 (comment)

Test plan

Tests added

@SimenB
Copy link
Member Author

SimenB commented Oct 15, 2021

I think this is ready now.

One issue is that if the user provides a packageFilter this overrides it... I guess I can call any user provided filter first then ours

@@ -6,13 +6,9 @@
"mjs",
"json"
],
"resolver": "<rootDir>/resolver.js",
Copy link
Member Author

@SimenB SimenB Oct 15, 2021

Choose a reason for hiding this comment

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

we don't need this anymore as this just tests that the condition node, browser etc of the main entry point are supported, and since we add support to jest itself for the entry point (and the things we test don't have a main), the rest works.

resolver: require.resolve('../__mocks__/userResolver'),
});

expect(mockResolveSync).toHaveBeenCalledWith(
Copy link
Member Author

Choose a reason for hiding this comment

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

since we now wrap the function, it's not actually passed as is through, so I had to make some changes so packageFilter is actually called and assert on that

@SimenB
Copy link
Member Author

SimenB commented Oct 15, 2021

Hmm, the change I had to make in the last commit worries me. This module used to work since there's a index.js file in root, but exports just has node and default (which is browser) conditions. We don't provide the node condition since it can mean require or import.

While this is probably fine in most cases, it might mean we must wait with this PR until Jest 28 as well...

Or at least we need to consider adding node condition as well, then passing the file we get back through the checks for whether it should be loaded as ESM or not and throw if it is.

EDIT: Some misunderstandings here - we should always check if file resolved from exports should be loaded as ESM or not, what condition it came from is irrelevant.

@SimenB SimenB force-pushed the exports-resolve-main branch from 142df6d to 601e494 Compare October 15, 2021 07:35
@codecov-commenter
Copy link

codecov-commenter commented Oct 15, 2021

Codecov Report

Merging #11919 (5c97bf3) into main (b5aec03) will increase coverage by 0.02%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #11919      +/-   ##
==========================================
+ Coverage   68.72%   68.74%   +0.02%     
==========================================
  Files         323      323              
  Lines       16631    16649      +18     
  Branches     4799     4805       +6     
==========================================
+ Hits        11429    11445      +16     
- Misses       5169     5171       +2     
  Partials       33       33              
Impacted Files Coverage Δ
packages/jest-resolve/src/defaultResolver.ts 88.00% <88.88%> (+2.28%) ⬆️

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 b5aec03...5c97bf3. Read the comment docs.

@SimenB SimenB force-pushed the exports-resolve-main branch from 601e494 to 96ee934 Compare October 17, 2021 16:47
@SimenB
Copy link
Member Author

SimenB commented Oct 17, 2021

OK, added a condition that if there is an index file in root, we also skip the resolution. That should make the change backwards compatible.

We need to pass the node condition, but that will also require changes to the runtime to check the format of the returned file. So I'll save that for v28 (which should have full support regardless)

@SimenB SimenB merged commit bc3c921 into jestjs:main Oct 17, 2021
@SimenB SimenB deleted the exports-resolve-main branch October 17, 2021 18:02
@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 Nov 17, 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