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: support of imports field #13723

Merged
merged 8 commits into from
Jan 5, 2023
Merged

fix: support of imports field #13723

merged 8 commits into from
Jan 5, 2023

Conversation

unional
Copy link
Contributor

@unional unional commented Jan 3, 2023

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
Copy link
Contributor Author

unional commented Jan 3, 2023

@SimenB 🍻

@unional unional marked this pull request as draft January 3, 2023 08:36
Copy link
Member

@SimenB SimenB left a comment

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
Copy link
Contributor Author

unional commented Jan 3, 2023

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

Updated changelog and removed copy.mjs
@unional
Copy link
Contributor Author

unional commented Jan 3, 2023

I think I know why the test fails.

The createResolveOptions():

function createResolveOptions(
  conditions: Array<string> | 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
Copy link
Member

SimenB commented Jan 3, 2023

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

@unional
Copy link
Contributor Author

unional commented Jan 3, 2023

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
Copy link
Member

SimenB commented Jan 3, 2023

What do you use for conditions if none are passed?

@unional
Copy link
Contributor Author

unional commented Jan 3, 2023

https://github.com/lukeed/resolve.exports/blob/master/src/index.js#L71-L73

resolve.exports is using a Set() and disregard the order in the conditions array.

I don't think that is correct as:

{
  "exports": {
    "node": {
      "import": "./a.js"
    },
    "import": {
      "node": "./b.js"
     }
  }
}

will get different result. I'll clarify that in the Node.js PR.

@SimenB
Copy link
Member

SimenB commented Jan 3, 2023

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
Copy link
Contributor Author

unional commented Jan 3, 2023

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
Copy link

If you're interested, I have an resolver for exports and imports, both tested exhaustively against Node.js for accuracy:
https://github.com/privatenumber/resolve-pkg-maps

@unional
Copy link
Contributor Author

unional commented Jan 4, 2023

If you're interested, I have an resolver for exports and imports, both tested exhaustively against Node.js for accuracy:

Thanks. I'm actually fixing this:

https://github.com/privatenumber/resolve-pkg-maps#why-do-the-apis-return-an-array-of-paths

The Node.js implementation does not work that way. It returns a single entry.
cyberuni/resolve.imports#24

The actual implementation in Node.js:
https://github.com/nodejs/node/blob/main/lib/internal/modules/esm/resolve.js#L424

@privatenumber
Copy link

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
Copy link
Contributor Author

unional commented Jan 4, 2023

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
Copy link
Contributor Author

unional commented Jan 4, 2023

@privatenumber Nicely written.

I spot a few differences:

  • 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 specificity compare (PATTERN_KEY_COMPARE) didn't assert for single *
  • The extra check for node_modules could be problematic (e.g. when working with yarn PnP and pnpm).

@privatenumber
Copy link

privatenumber commented Jan 4, 2023

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

  • The specificity compare (PATTERN_KEY_COMPARE) didn't assert for single *

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

  • The extra check for node_modules could be problematic (e.g. when working with yarn PnP and pnpm).

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
Copy link
Contributor Author

unional commented Jan 4, 2023

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
Copy link
Contributor Author

unional commented Jan 4, 2023

@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
Copy link
Contributor Author

unional commented Jan 4, 2023

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
Copy link

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.

resolve-pkg-maps handles this and there's a test-case for it to enforce the same error as Node.js is thrown:

https://github.com/privatenumber/resolve-pkg-maps/blob/3817652f2817beb8a7428c6b97dabe0cd8b96bd4/tests/exports/star.ts#L135-L158


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)

I'm not completely sure what you mean, but resolve-pkg-maps has test cases to enforce node_modules is valid in imports key:
https://github.com/privatenumber/resolve-pkg-maps/blob/3817652f2817beb8a7428c6b97dabe0cd8b96bd4/tests/imports/index.ts#L22-L38

Is there a test-case you have in mind?


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.

This is handled by resolve-pkg-maps too, and is capable of handling both Node.js and Webpack/TypeScript at the same time.

Test-case:
https://github.com/privatenumber/resolve-pkg-maps/blob/3817652f2817beb8a7428c6b97dabe0cd8b96bd4/tests/exports/fallback-array.ts#L7-L26

Demo:
https://stackblitz.com/edit/node-pbjnfn?file=exports.ts

A lot of what you're pointing out is already covered in the tests and easy to verify on your end.

@unional
Copy link
Contributor Author

unional commented Jan 4, 2023

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
Copy link
Member

SimenB commented Jan 4, 2023

@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
Copy link

privatenumber commented Jan 4, 2023

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
Copy link
Contributor Author

unional commented Jan 4, 2023

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.

fixing the error message issue
@unional
Copy link
Contributor Author

unional commented Jan 5, 2023

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

@SimenB
Copy link
Member

SimenB commented Jan 5, 2023

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

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks!

@SimenB SimenB merged commit 499fdf0 into jestjs:main Jan 5, 2023
@unional unional deleted the imports-field branch January 5, 2023 10:23
@lukeed
Copy link
Contributor

lukeed commented Jan 15, 2023

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

@SimenB
Copy link
Member

SimenB commented Jan 15, 2023

Exciting!

@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 Feb 15, 2023
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]: Resolve imports field
5 participants