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: pass conditions when resolving modules #11859

Merged
merged 11 commits into from
Sep 13, 2021
Merged

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Sep 9, 2021

Summary

A start on implementing support for package exports (#9771). This should unblock people when writing their own resolvers. We'll need to add builtin support in the future, but at least unblocking and letting people write their own and publish is a great start.

It is now correctly passing through a conditions array, but there is some caching in either jest-resolve or jest-haste-map (or someplace else...) that returns the wrong module back, meaning CJS tries to load ESM and fails. Running the test files one by one always works, as does running with --no-cache. Need to investigate, but I'm out of time (& energy) for now.

Test plan

Test added

@@ -173,6 +173,9 @@ const supportsNodeColonModulePrefixInImport = (() => {
return stdout === 'true';
})();

const esmConditions = ['import', 'require', 'default'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Node.js doesn't pass require for ESM: https://nodejs.org/api/packages.html#packages_conditional_exports ("import" and "require" are mutually exclusive).

Copy link
Member Author

Choose a reason for hiding this comment

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

Even though import supports both? I guess the idea is to fallback to default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or just node I guess

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it fallbacks to either default or node.

Copy link
Member Author

@SimenB SimenB Sep 9, 2021

Choose a reason for hiding this comment

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

Should i remove it or pass node you think? (Still somewhat worried about that browser thing)

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 removed it for now, let's see how that works out 🙂 Better to have too little than too much

@SimenB
Copy link
Member Author

SimenB commented Sep 10, 2021

This bugged me too much, so I neglected work today to work on this 😅 Managed to fix the caching issue, so this just needs docs now

const moduleID = this._resolver.getModuleID(
this._virtualMocks,
from,
moduleName,
isInternal ? undefined : {conditions: cjsConditions},
Copy link
Member Author

@SimenB SimenB Sep 10, 2021

Choose a reason for hiding this comment

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

this is wrong and just to avoid the fact pathFilter doesn't really work for what we want (it asks for every relative import within a package as well). Should probably rework the test, but since this is internal loading it doesn't really matter up until we actually support conditions out of the box.

I do think we shouldn't use user-provided resolvers when loading our own stuff, but it is what it is

@@ -294,6 +298,9 @@ export default class Runtime {
const moduleID = this._resolver.getModuleID(
this._virtualMocks,
filePath,
undefined,
// is this correct?
Copy link
Member Author

Choose a reason for hiding this comment

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

automock is weird, and this should be a fully resolved path already, so I guess we can remove it as it doesn't matter

@@ -173,7 +173,7 @@ const supportsNodeColonModulePrefixInImport = (() => {
return stdout === 'true';
})();

// consider "node" condition as well - maybe switching on `global.window` in the test env?
// consider `node` & `browser` condition as well - maybe switching on `global.window` in the test env?
Copy link
Contributor

@nicolo-ribaudo nicolo-ribaudo Sep 10, 2021

Choose a reason for hiding this comment

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

Maybe they could be enabled by jest-environment-* packages? I don't know if the internal architecture makes it impossible, but from my (external, user) point of view it's what makes more sense.

Copy link
Member Author

@SimenB SimenB Sep 10, 2021

Choose a reason for hiding this comment

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

Definitely a possibility! Main worry is browser meaning either ESM or CJS, but I guess we can add the option and just not use it ourselves for now

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'll open up a separate PR with that after landing this 🙂

@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2021

Codecov Report

Merging #11859 (8a7f59a) into main (03ff659) will decrease coverage by 0.01%.
The diff coverage is 55.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #11859      +/-   ##
==========================================
- Coverage   68.95%   68.93%   -0.02%     
==========================================
  Files         312      312              
  Lines       16400    16411      +11     
  Branches     4750     4759       +9     
==========================================
+ Hits        11308    11313       +5     
- Misses       5065     5071       +6     
  Partials       27       27              
Impacted Files Coverage Δ
packages/jest-resolve/src/defaultResolver.ts 88.67% <ø> (ø)
packages/jest-resolve/src/resolver.ts 55.86% <22.22%> (-0.33%) ⬇️
packages/jest-runtime/src/index.ts 57.38% <70.00%> (+0.04%) ⬆️
packages/expect/src/utils.ts 95.58% <0.00%> (-0.56%) ⬇️

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 03ff659...8a7f59a. Read the comment docs.

@SimenB SimenB marked this pull request as ready for review September 10, 2021 10:30
@IanVS
Copy link
Contributor

IanVS commented Sep 10, 2021

This is going to be a bit of a gnarly rebase for #11540. I don't suppose we can hold off on doing more work on the jest-resolver until that one is in, could we? Or, if you don't think that 11540 will land, I'll go ahead and close it out and stop trying to keep it up-to-date.

Edit, after looking a little closer, it hopefully won't be too bad to rebase. But it would be nice to not keep spending time rebasing and fixing merge conflicts. ;)

@SimenB
Copy link
Member Author

SimenB commented Sep 11, 2021

@IanVS I do have plans to merge that PR, and I sympathise with having to keep it up to date. I don't think there'll be any further changes to the resolver after this PR (unless there are bugs). Adding support for node: prefix and conditions have been highly requested for some time, and beyond proper exports support (which I don't plan to tackle right now) there's nothing in the pipeline I'm aware of that will touch this logic 🙂

@SimenB SimenB merged commit a0bf671 into jestjs:main Sep 13, 2021
@SimenB SimenB deleted the conditions branch September 13, 2021 07:05
@Manubi
Copy link

Manubi commented Sep 17, 2021

Hi,
I tried to implement it myself but just stumble from error to error. Can anyone give me a hint on how to implement it? My issue is described here. Thanks!

@SimenB
Copy link
Member Author

SimenB commented Sep 17, 2021

I'd assume #9771 (comment) works fine, but passing conditionNames: conditions instead of hard-coding a set

@nicolo-ribaudo
Copy link
Contributor

@SimenB I don't know if it's expected, but I'm getting options.conditions: undefined when resolving the files for setupFilesAfterEnv. I have to use options.conditions || ["default"] to make enhanced-resolve works.

Maybe this is expected, but it would be nice if Jest defaulted to ["default"].

@SimenB
Copy link
Member Author

SimenB commented Sep 17, 2021

@nicolo-ribaudo Hmm, good point. The thing is we resolve those files when normalizing configuration (i.e. way before actually loading them for individual test files, and before loading e.g. testEnvironment). So we cannot really know which conditions will be "correct".

We might need to pass whatever path is given to us through in Jest rather that resolving in jest-config when loading the config, and then resolve the files when we're actually loading them instead. That sounds like it might need to go in a major though as the path would potentially stop being absolute for people consuming the config directly from jest-config.

One thing we could do (if we don't already) is to skip the resolver call if the path is absolute - I don't there's any need for us to resolve again in those cases?

@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 Oct 18, 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