fix: support of imports field by unional · Pull Request #13723 · jestjs/jest (original) (raw)

@unional

Summary

use resolve.imports to handle imports field.

It supports more patterns supported by Node.js and closely tracks the actual implementation within Node.js.

fixes #13707

Test plan

Added a few tests, including nested pattern and array pattern.

@unional

unional

unional

@unional

SimenB

SimenB

Choose a reason for hiding this comment

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

can you remove the copy.mjs in root and add a link to this PR in the changelog entry I added in #13705?

@unional

can you remove the copy.mjs in root and add a link to this PR in the changelog entry I added in #13705?

Sure, don't know why that copy.mjs got created again... I deleted it before commit. Will fix and also update changelog

@unional

Updated changelog and removed copy.mjs

SimenB

@unional

I think I know why the test fails.

The createResolveOptions():

function createResolveOptions( conditions: Array | undefined, ): ResolveExportsOptions { return conditions ? {conditions, unsafe: true} : // no conditions were passed - let's assume this is Jest internal and it should be require {browser: false, require: true}; }

Is specific to the resolve.exports call.. The unsafe, browser and require are specific to that. Likely to handle legacy import. The resolve.imports implements the PACKAGE_IMPORTS_RESOLVE which is for ESM. Does not handle those options.

@SimenB

Those options are just for specifying conditions, nothing to do with "legacy" or not

SimenB

@unional

@unional

Those options are just for specifying conditions, nothing to do with "legacy" or not

I checked resolve.exports code. Those flags are specific to that lib. I'll fix this here.

@SimenB

What do you use for conditions if none are passed?

@unional

@SimenB

Order of conditions shouldn't matter, as we use the order of keys in package.json. First match wins.

https://nodejs.org/api/packages.html#conditional-exports

Within the "exports" object, key order is significant. During condition matching, earlier entries have higher priority and take precedence over later entries. The general rule is that conditions should be from most specific to least specific in object order.

The order of entries in the conditions array (or Set) makes no difference to resolution.

@unional

Order of conditions shouldn't matter, as we use the order of keys in package.json. First match wins.

Yes, for conditional exports it is checking by the key order. But for the nested condition case, it should matter, IMO.

My gut feeling is that this is a limitation due to legacy code. Likely nested conditions are added later on so can't change ship.

nodejs/node#46068 (comment)

I'll update resolve.imports to do the same. That should fix the issue here.

@privatenumber

@unional

@privatenumber

You may have missed this part.

If you want Node.js behavior, you can just use the first element in the array.

The array is for use-cases where users may want to implement TypeScript or Webpack behavior.

@unional

You may have missed this part.

Yeah, just read that. Let me check. That is a good take.

But based on what I understand, that doesn't work as the array can be:

[{ "node": "./node.js" }, "./browser.js"]

Which the caller should not handle.

@unional

@privatenumber Nicely written.

I spot a few differences:

@privatenumber

The user should correct the message via try-catch if they want.
The library doesn't have file-system access so it doesn't have enough information to construct an error message identical to Node.js.

I'm not sure what this means. Can you elaborate?

That logic is from Node.js:
https://github.com/nodejs/node/blob/v18.8.0/lib/internal/modules/esm/resolve.js#L371

Their regex is a little cryptic because they cover encoded values.

But it would still work with PnP because imports can only contain bare-specifiers (package names) as opposed to node_modules paths.

@unional

I'm not sure what this means. Can you elaborate?

PATTERN_KEY_COMPARE(keyA, keyB)

Assert: keyA ends with "/" or contains only a single "*".
Assert: keyB ends with "/" or contains only a single "*".

This is what I'm referring to.
And the algorithm is actually outdated. I'm updating it here: nodejs/node#46068

Their regex is a little cryptic because they cover encoded values

I think it is for resolving the target, not the key/specifier. so I think the resolver code should not need to check that. It's a different concern. (also it's a question on which code is responsible of what. in resolve.imports I'm trying to handle just the PACKAGE_IMPORT_RESOLVE()` functionality as much as possible, leaving other code handles other stuff)

And also about the array pattern, I think it should be handled by the resolver:

the array can be:

[{ "node": "./node.js" }, "./browser.js"]

Which the caller should not handle.

So yeah, it sucks that webpack and typescript are handling it differently. So it seems like there is no way to support them and Node.js at the same time.

@unional

@SimenB any suggestion on this test? In my own assertron library, I can do a.throws(() => {...}, (e) => e.message.startsWith(...)).

But I don't know how to do that with expect() in jest.

The error message contains the package.json and base file path, meaning it will be different in CI and everyone's environment.

    test('fails for non-existent mapping', () => {
      // jest `toThrow()` does not take a validator,
      // so can't use msg.startWith() test.
      expect(() => {
        Resolver.findNodeModule('#something-else', {
          basedir: path.resolve(importsRoot, './foo-import/index.js'),
          conditions: [],
        });
      }).toThrow();
    });

@unional

I have fixed most of the errors except one. Both resolve.imports and resolve-pkg-maps failed this test:

 FAIL  packages/jest-runtime/src/__tests__/runtime_require_module.test.js (7.369 s)
  ● Runtime requireModule › resolves platform extensions based on the default platform

    Cannot find module 'Platform' from 'root.js'

      at Resolver._throwModNotFoundError (packages/jest-resolve/build/resolver.js:427:11)
          at async Promise.all (index 3)

Will be checking the current changes in, and continue to look at that one.

@privatenumber

@unional

@unional

@unional

Oh, the test actually passes in CI. Seems like a WSL issue locally then.

@unional unional marked this pull request as ready for review

January 4, 2023 07:24

@SimenB

@SimenB

SimenB

@SimenB

@privatenumber I really like the look of your package - a single one support both imports and exports seems very reasonable to me. 👍 Definitely something we should consider using. Is that a package that will be used by other packages in the near future? (I had faith in resolve.exports at the time since Yarn was implementing their exports support using it)

  • the errors does not extends from TypeError and since the input is simplified, the error messages is not the same as in Node.js (need the package.json folder and the base input)

The user should correct the message via try-catch if they want. The library doesn't have file-system access so it doesn't have enough information to construct an error message identical to Node.js.

Would you be up for adding options for the user to pass in the needed data so you can build up the matching error message?

@privatenumber

Is that a package that will be used by other packages in the near future? (I had faith in resolve.exports at the time since Yarn was implementing their exports support using it)

I implemented it for the TypeScript resolver I'm working on for tsx. The TS resolver is still in-progress but the exports/imports resolver is ready to use.

Would you be up for adding options for the user to pass in the needed data so you can build up the matching error message?

Sure, I'm open to discussing this.

@unional

When I working on this I also tried to use resolve-pkg-maps to see the difference. Tests also passed. One thing I notice is that the type doesn't match.

The JSONTypes need to be casted before passing in. Maybe the param type needs to be loosen.

@unional

fixing the error message issue

@unional

Do you want to change this PR to use resolve-pkg-maps or do it in the next PR?

@SimenB

Do you want to change this PR to use resolve-pkg-maps or do it in the next PR?

we can do it in a separate PR and remove resolve.exports at the same time

SimenB

Choose a reason for hiding this comment

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

thanks!

@lukeed

This is now supported in resolve.exports@next and will be released as 2.0 stable in the next few days

@SimenB

@github-actions

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

Feb 15, 2023

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})