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(pnp): exports support #2431

Merged
merged 41 commits into from
Feb 26, 2021
Merged

feat(pnp): exports support #2431

merged 41 commits into from
Feb 26, 2021

Conversation

paul-soporan
Copy link
Member

@paul-soporan paul-soporan commented Feb 2, 2021

What's the problem this PR addresses?

I'm opening this as a draft so that people know that I'm working on it (and it's nearly finished).

This PR adds support for the "exports" field to PnP (CJS only, @merceyz is looking into ESM in #2161) via the resolve.exports package.

Closes #650.

Edit: Forgot to give credit earlier, but this PR adapts some of @bgotink's code from #1359.

TODO:

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@paul-soporan paul-soporan marked this pull request as draft February 2, 2021 21:44
Comment on lines 223 to 229
const resolvedExport = resolveExport(pkgJson, ppath.normalize(subpath), {
browser: false,
require,
// TODO: implement support for the --conditions flag
// Waiting on https://github.com/nodejs/node/issues/36935
conditions,
});
Copy link
Member

Choose a reason for hiding this comment

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

I mentioned it on Discord but just to have it here as well so it isn't forgotten:

No matter what we pass here resolve.exports forces two conditions from ['require', 'import', 'browser', 'node'] to always be added while we need it to only use the conditions we pass so we have full control. For example if react-native were to use this to resolve native stuff it should not get matches for neither node nor browser. While in the ESM loader the conditions can be anything some other loader passes so the same applies there.

cc @lukeed

Copy link

Choose a reason for hiding this comment

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

That is correct. See defaultConditions and also here. Node is always juggling up to 4 conditions (with 2 forced as defaults).

You can add whatever custom conditions you want to support, but at the end of the day, the specified key order determines the matching priority. A RN user will need to do something like this for their package to work anywhere (not just my module):

{
  "exports": {
    "react-native": {
      "import": "./rn/index.mjs",
      "require": "./rn/index.cjs"
    },
    "import": "./...",
     //...
  }
}

And then specify the "react-native" condition

Copy link

Choose a reason for hiding this comment

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

Seems unfortunate that node's defaults should be forced on everybody using exports field. For a module that isn't supported to run in Node, it should just fail to resolve or something (or falling back to default) not pick up node.

There's already a switch in the library for browser condition so it's not consistently enforced - why not just fully let the caller provide their own conditions (barring possibly default - that seems reasonable to always add)? I.e. remove the two allows.add calls (and thus the options leaving only conditions). Or probably add node if no conditions is passed, but otherwise add nothing.


I was gonna open up an issue over in resolve.exports, but since I came over this PR with this comment before I got to it, I sneakily commented here instead 😀 Happy to open up an issue over there instead

Copy link

@SimenB SimenB Oct 4, 2021

Choose a reason for hiding this comment

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

As another datapoint, TypeScript could not have used resolve.exports to implement support for types condition with their official example: https://devblogs.microsoft.com/typescript/announcing-typescript-4-5-beta/#packagejson-exports-imports-and-self-referencing

I think resolve.exports should be implementation/host independent (maybe a resolve.exports-node or just a import { node } from 'resolve.exports' makes sense, but the base export shouldn't put these limitations there)

Copy link

Choose a reason for hiding this comment

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

@arcanis
Copy link
Member

arcanis commented Feb 13, 2021

Tests almost pass, but need to be adjusted for Windows support 👍

@paul-soporan paul-soporan marked this pull request as ready for review February 24, 2021 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Case Study] Exports Support
5 participants