feat: pass `conditions` when resolving modules #11859

SimenB · 2021-09-09T14:17:32Z

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

nicolo-ribaudo · 2021-09-09T14:52:03Z

packages/jest-runtime/src/index.ts

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

+const esmConditions = ['import', 'require', 'default'];


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

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

Or just node I guess

Yes, it fallbacks to either default or node.

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

I removed it for now, let's see how that works out 🙂 Better to have too little than too much

SimenB · 2021-09-10T08:32:17Z

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

SimenB · 2021-09-10T08:35:08Z

packages/jest-runtime/src/index.ts

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


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

SimenB · 2021-09-10T08:44:37Z

packages/jest-runtime/src/index.ts

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


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

nicolo-ribaudo · 2021-09-10T08:49:49Z

packages/jest-runtime/src/index.ts

@@ -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?


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.

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

I'll open up a separate PR with that after landing this 🙂

codecov-commenter · 2021-09-10T10:22:16Z

Codecov Report

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

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

IanVS · 2021-09-10T22:03:32Z

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 · 2021-09-11T13:03:14Z

@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 🙂

Manubi · 2021-09-17T09:19:50Z

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 · 2021-09-17T09:25:41Z

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

nicolo-ribaudo · 2021-09-17T12:32:57Z

@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 · 2021-09-17T14:24:33Z

@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 · 2021-10-18T00:04:25Z

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.

feat: pass conditions when resolving modules

260eaaa

facebook-github-bot added the cla signed label Sep 9, 2021

SimenB marked this pull request as draft September 9, 2021 14:17

SimenB mentioned this pull request Sep 9, 2021

Support package exports in jest-resolve #9771

Closed

ci

839edc0

nicolo-ribaudo reviewed Sep 9, 2021

View reviewed changes

SimenB added 2 commits September 10, 2021 10:18

do not pass require condition for esm

0f29eb5

add resolve options to cache key

127071a

SimenB commented Sep 10, 2021

View reviewed changes

snaps again

9afb6ef

SimenB commented Sep 10, 2021

View reviewed changes

resolve comment

30b8beb

nicolo-ribaudo reviewed Sep 10, 2021

View reviewed changes

oops

980ff26

SimenB added 2 commits September 10, 2021 12:25

lint

03abc32

docs

b7e2b20

SimenB marked this pull request as ready for review September 10, 2021 10:30

SimenB added 2 commits September 10, 2021 12:42

changelog

f3be5cd

prettier

8a7f59a

SimenB requested review from jeysal, thymikee and cpojer September 10, 2021 10:50

SimenB mentioned this pull request Sep 10, 2021

feat: support conditions from test environments #11863

Merged

SimenB merged commit a0bf671 into jestjs:main Sep 13, 2021

SimenB deleted the conditions branch September 13, 2021 07:05

nicolo-ribaudo mentioned this pull request Sep 17, 2021

Update Jest to 27.2.0 babel/babel#13776

Merged

github-actions bot locked as resolved and limited conversation to collaborators Oct 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: pass `conditions` when resolving modules #11859

feat: pass `conditions` when resolving modules #11859

SimenB commented Sep 9, 2021 •

edited

nicolo-ribaudo Sep 9, 2021

SimenB Sep 9, 2021

SimenB Sep 9, 2021

nicolo-ribaudo Sep 9, 2021

SimenB Sep 9, 2021 •

edited

SimenB Sep 10, 2021

SimenB commented Sep 10, 2021

SimenB Sep 10, 2021 •

edited

SimenB Sep 10, 2021

nicolo-ribaudo Sep 10, 2021 •

edited

SimenB Sep 10, 2021 •

edited

SimenB Sep 10, 2021

codecov-commenter commented Sep 10, 2021 •

edited

IanVS commented Sep 10, 2021 •

edited

SimenB commented Sep 11, 2021

Manubi commented Sep 17, 2021

SimenB commented Sep 17, 2021

nicolo-ribaudo commented Sep 17, 2021

SimenB commented Sep 17, 2021 •

edited

github-actions bot commented Oct 18, 2021

feat: pass conditions when resolving modules #11859

feat: pass conditions when resolving modules #11859

Conversation

SimenB commented Sep 9, 2021 • edited

Summary

Test plan

nicolo-ribaudo Sep 9, 2021

Choose a reason for hiding this comment

SimenB Sep 9, 2021

Choose a reason for hiding this comment

SimenB Sep 9, 2021

Choose a reason for hiding this comment

nicolo-ribaudo Sep 9, 2021

Choose a reason for hiding this comment

SimenB Sep 9, 2021 • edited

Choose a reason for hiding this comment

SimenB Sep 10, 2021

Choose a reason for hiding this comment

SimenB commented Sep 10, 2021

SimenB Sep 10, 2021 • edited

Choose a reason for hiding this comment

SimenB Sep 10, 2021

Choose a reason for hiding this comment

nicolo-ribaudo Sep 10, 2021 • edited

Choose a reason for hiding this comment

SimenB Sep 10, 2021 • edited

Choose a reason for hiding this comment

SimenB Sep 10, 2021

Choose a reason for hiding this comment

codecov-commenter commented Sep 10, 2021 • edited

Codecov Report

IanVS commented Sep 10, 2021 • edited

SimenB commented Sep 11, 2021

Manubi commented Sep 17, 2021

SimenB commented Sep 17, 2021

nicolo-ribaudo commented Sep 17, 2021

SimenB commented Sep 17, 2021 • edited

github-actions bot commented Oct 18, 2021

feat: pass `conditions` when resolving modules #11859

feat: pass `conditions` when resolving modules #11859

SimenB commented Sep 9, 2021 •

edited

SimenB Sep 9, 2021 •

edited

SimenB Sep 10, 2021 •

edited

nicolo-ribaudo Sep 10, 2021 •

edited

SimenB Sep 10, 2021 •

edited

codecov-commenter commented Sep 10, 2021 •

edited

IanVS commented Sep 10, 2021 •

edited

SimenB commented Sep 17, 2021 •

edited