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(node-resolve): support package entry points #540

Merged

Conversation

LarsDenBakker
Copy link
Contributor

Rollup Plugin Name: node-resolve

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers: Fixes #362

Description

This implements package entrypoints as specified in https://nodejs.org/api/esm.html#esm_package_entry_points.

For any bare import that is resolved we first look up the package.json. If the package.json has an export map we use that for resolving instead of the regular node resolve logic. If a path is not found in the export map, an error is thrown. I've tried to match the implementation of node, and also throw the same informative error messages as node js to ensure a consistent experience. @guybedford I would appreciate it if you would be able to verify the implementation. Consistency is very important here.

We need make a decision on how to the exports field should relate to other main fields and the browser field. The way I implemented it right if a package has package entrypoints defined we ignore any main fields configuration, but we do allow the browser field to override the export map result. This isn't really a conscious decision, it's just a result of how the code is structured right now.

This is a breaking change because it changes the way modules are resolved.

@LarsDenBakker LarsDenBakker force-pushed the feat/node-resolve-conditional-exports branch from 6f4fa45 to b12b0f7 Compare August 13, 2020 18:18
@LarsDenBakker LarsDenBakker force-pushed the feat/node-resolve-conditional-exports branch 2 times, most recently from c107aae to 21f76b7 Compare August 13, 2020 18:31
Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

I am very excited to have a deeper look into this. One question up front: Did you consider interaction with the mainFields option because it is not clear to me. Is exports always taking precedence? Is there a way to make any other field take precedence for the main import or to disable exports resolution? In any case the documentation should be clear what is the precedence between the different resolution options.

packages/node-resolve/src/index.js Outdated Show resolved Hide resolved
@LarsDenBakker
Copy link
Contributor Author

LarsDenBakker commented Aug 13, 2020

@lukastaegert in my opinion the whole main fields thing is very confusing to most developers. They try to tweak the node resolve config until it "just works". Package entrypoints is a clean break, and perhaps the best chance we have right now at a standard format for describing the export structure of a package. Therefore I think it's best that if a package has specified exports it should take priority over main fields.

We could add options to disable or tweak the behavior, but I'd like to avoid that if possible. You can't disable this behavior in node, making this logic consistent is important I think. Package authors know best what applied to what environment. Power users can write a plugin with resolveId if they really need to customize it.

@lukastaegert
Copy link
Member

Power users can write a plugin with resolveId if they really need to customize it.

Makes sense. Still I think it would be sensible to add a note to the documentation of the mainFields option that it will be ignored for packages that have an exports field.

@LarsDenBakker LarsDenBakker force-pushed the feat/node-resolve-conditional-exports branch from 21f76b7 to 2deb375 Compare August 13, 2020 20:00
@LarsDenBakker
Copy link
Contributor Author

Yeah, good point. I've added it in the readme.

@shellscape
Copy link
Collaborator

A note from the sidelines to everyone participating: Please do make sure to let us know when this is ready! There are a lot of moving parts to this and several simultaneous conversations, and I want to make sure it doesn't go stale or get missed. 🍻

@LarsDenBakker
Copy link
Contributor Author

I just realized I'm missing support for exporting entire directories:

{
  "exports": {
    "./lib/": "./lib/",
  }
}

@LarsDenBakker LarsDenBakker force-pushed the feat/node-resolve-conditional-exports branch 2 times, most recently from 5959a48 to 9ab26cd Compare August 22, 2020 21:54
@LarsDenBakker
Copy link
Contributor Author

I added support for directory exports.

@LarsDenBakker
Copy link
Contributor Author

It would be great to get some 👀 on this. @guybedford since you worked on this on the node side, would you be able to double check this?

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

@LarsDenBakker left a couple of intial comments, but haven't done a thorough review.

If you care about correctness, the exact details are fully available in the resolution algorithm described in https://nodejs.org/dist/latest-v14.x/docs/api/esm.html#esm_resolution_algorithm (expand the details).

Also implement the "imports" field and self-resolution :)

### `exportConditions`

Type: `Array[...String]`<br>
Default: `['module', 'import']`
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is "module" defined here? I don't know exactly what its definition is or where that is supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the standard field webpack and rollup will be using.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand it is a useful way to enable CommonJS modules to load ES modules to avoid duplication cases (although these pains are yet to be felt as there dont seem to be that many people shipping dual mode packages that hit these cases yet).

But I'm not sold that it is necessarily the best way to do this, not that I can't be :)

I just don't think RollupJS should ship this without the meaning being very clearly defined for the various use cases.

Let me just try to get the definition straight then at least:

  1. Does "module" imply modern syntax, or is that not part of its definition?
  2. Does "module" imply resolution in a bundler like Webpack or Rollup only? Or is it expected that the definition extends to other bundlers or tools?
  3. Does "module" apply for the Node.js platform target? This can be seen then as something that changes the default Node.js semantics under bundling which breaks the overall goal of RollupJS building for Node as retaining semantics.
  4. Does "module" apply for any other platforms other than the browser?
  5. Finally, do you think Node.js itself should implement the "module" field when resolving import (and then just not when resolving require)? This seems like it might avoid the repetition of defining "exports": { "import": "./main.mjs", "module": "./main.mjs", "require": "./main.cjs" } current redundancy for these sorts of packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

import can point to es module which wrap commonjs modules. This is a problem for pure es module workflows, so there needs to be a way to indicate pure es modules. The name module came out of the discussion at #362 and webpack/webpack#11014 (comment)

I'm fine with deferring that to a later PR, but it would be a breaking change later on again. @lukastaegert what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I'm not against it, I just want to be clear on the meaning. Ok, it seems the key source here is Tobias's proposal. Reading his proposal more clearly, let me try and answer my own questions then:

Does "module" imply modern syntax, or is that not part of its definition?

Not mentioned in the doc

Does "module" imply resolution in a bundler like Webpack or Rollup only? Or is it expected that the definition extends to other bundlers or tools?

Not mentioned in the doc.

Does "module" apply for the Node.js platform target? This can be seen then as something that changes the default Node.js semantics under bundling which breaks the overall goal of RollupJS building for Node as retaining semantics.

The doc explicitly mentions that you would want to use it when bundling for a Node.js target so it doesn't seem to be Node or browser specific at all as far as I can tell.

Does "module" apply for any other platforms other than the browser?

As above, it can apply to Node.js

Finally, do you think Node.js itself should implement the "module" field when resolving import (and then just not when resolving require)? This seems like it might avoid the repetition of defining "exports": { "import": "./main.mjs", "module": "./main.mjs", "require": "./main.cjs" } current redundancy for these sorts of packages.

Node.js likely would not do this.

Basically it seems that "module" is specifically there to solve the bundler problem for when CJS is able to import from ESM in bundlers only. And RollupJS as a bundler can join Webpack in doing that.

These constraints seem clear to me, I hope we can give users a little guidance on when to use "module" mainly.

@lukastaegert will entirely defer to you on this one, I'm happy either way, just erring on the side of caution in introducing new things.

Copy link
Member

Choose a reason for hiding this comment

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

As you correctly stated, this condition was specifically created for the bundler use case and I definitely think we should add it by default. An entirely different question is when library authors should use it. Here, I would see it as a last resort solution to solve e.g. certain dual state hazard situations in a purely ESM way, so there are other patterns I would recommend first.

packages/node-resolve/src/resolveId.js Outdated Show resolved Hide resolved
@LarsDenBakker
Copy link
Contributor Author

Thanks! I didn't see the algorithm before, I will double check if I covered everything there.

I think we can take the imports field in a separate PR. Definitely doesn't make resolving imports simpler though :(

@guybedford
Copy link
Contributor

@LarsDenBakker I was only pointing you to it for the long-term roadmap. It's a fringe feature only needed in esoteric use cases like future builtin polyfills.

@guybedford
Copy link
Contributor

@LarsDenBakker hope my feedback wasn't stalling here, this would be very nice to merge.

@LarsDenBakker LarsDenBakker force-pushed the feat/node-resolve-conditional-exports branch 2 times, most recently from d610629 to 6caeeaa Compare September 12, 2020 16:28
@LarsDenBakker
Copy link
Contributor Author

LarsDenBakker commented Sep 12, 2020

@guybedford no worries, I didn't have the time yet to look at your comments :)

I've updated the PR now and rebased on master. I think we're good to go here.

@LarsDenBakker LarsDenBakker force-pushed the feat/node-resolve-conditional-exports branch 2 times, most recently from 784ebc7 to 9be1b28 Compare September 12, 2020 18:40
@lukastaegert
Copy link
Member

I just went through https://nodejs.org/api/packages.html#packages_package_entry_points. So it appears nearly everything is working awesomely except pattern conditions like "./foo/*": "./bar/*.js".

Not sure if we want to add them here but they seem very useful.

@LarsDenBakker LarsDenBakker force-pushed the feat/node-resolve-conditional-exports branch 2 times, most recently from abffd9e to 6f8227c Compare October 31, 2020 10:06
@LarsDenBakker
Copy link
Contributor Author

The star pattern was added to node after this PR was opened :) I added it now.

I updated the documentation as well.

@LarsDenBakker LarsDenBakker force-pushed the feat/node-resolve-conditional-exports branch from 6f8227c to 05c9ebc Compare October 31, 2020 10:10
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Left a few implementation comments.

Interesting to see the difference in implementation assumptions around subpath patterns in exports... if you have any feedback further please do share.

packages/node-resolve/README.md Outdated Show resolved Hide resolved
packages/node-resolve/README.md Outdated Show resolved Hide resolved
packages/node-resolve/src/resolveImportSpecifiers.js Outdated Show resolved Hide resolved
packages/node-resolve/src/resolveImportSpecifiers.js Outdated Show resolved Hide resolved
@LarsDenBakker
Copy link
Contributor Author

@guybedford thanks for the review!

@LarsDenBakker LarsDenBakker force-pushed the feat/node-resolve-conditional-exports branch from ead1189 to 9fe41a0 Compare October 31, 2020 20:46
packages/node-resolve/types/index.d.ts Outdated Show resolved Hide resolved
@LarsDenBakker LarsDenBakker force-pushed the feat/node-resolve-conditional-exports branch from 9fe41a0 to 3d54780 Compare November 2, 2020 22:20
Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Great work! This is ok from my side, but we should really get rid of the .only :)

packages/node-resolve/test/browser.js Outdated Show resolved Hide resolved
Co-authored-by: Lukas Taegert-Atkinson <lukastaegert@users.noreply.github.com>
@shellscape shellscape merged commit 3d60158 into rollup:master Nov 7, 2020
apexskier added a commit to apexskier/nova-extension-utils that referenced this pull request Jan 19, 2021
This fixes some issues when using rollup's node resolve 11

rollup/plugins#540
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[node-resolve] Support conditional exports defined in package.json
6 participants