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 pkg imports and export array #693

Merged

Conversation

LarsDenBakker
Copy link
Contributor

@LarsDenBakker LarsDenBakker commented Dec 6, 2020

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: #670

Description

To fix #670 I started reviewing the algorithm at https://nodejs.org/dist/latest-v15.x/docs/api/esm.html#esm_resolver_algorithm_specification and figured it would be better to implement that more closely.

This PR re-implements the whole package exports feature, basing it on the spec. This fixes the reported bug, and also adds support for the package imports feature defined at https://nodejs.org/api/packages.html#packages_subpath_imports

Copy link
Collaborator

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

Took me a while to digest all of it, but it looks good to me.

Copy link
Member

@tjenkinson tjenkinson left a comment

Choose a reason for hiding this comment

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

This wasn’t a thorough review. Just a few things I noticed skimming through.

Looks good though

@LarsDenBakker LarsDenBakker force-pushed the feat/node-resolve-improvements branch 2 times, most recently from ba0262b to 408b700 Compare December 9, 2020 17:17
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.

Incredible work, thanks again @LarsDenBakker.

@ctavan
Copy link
Contributor

ctavan commented Dec 9, 2020

@LarsDenBakker just out of curiosity: Did you check the efforts in browserify/resolve#224 ? Could that be re-used?

@LarsDenBakker
Copy link
Contributor Author

LarsDenBakker commented Dec 10, 2020

@ctavan yes potentially. that PR has been open for a long time though.

@remorses
Copy link

remorses commented Dec 10, 2020

Shouldn’t this logic live in the resolve package? Will you delete this code after resolve implements the exports logic?

@LarsDenBakker
Copy link
Contributor Author

LarsDenBakker commented Dec 11, 2020

Like I said above... maybe. I think the resolve package intends to implement commonjs-style resolve, not node-js es module style resolve. The rollup plugin also has some custom options, we need to see if these things can work together. But it would be better to have one implementation, also with webpack which also has it's own implementation.

Note that this PR is an upgrade of functionality that is already in the node-resolve plugin, a PR that I started back in august: #540. At the time I wasn't aware of the work being done in the resolve package for this. Now the original PR is merged, and I'm just making sure we implement the spec correctly. It's an important feature for the ecosystem, and it's important to be compatible with node and webpack here. I'm fine with deleting my code later.

@ExE-Boss
Copy link

ExE-Boss commented Dec 12, 2020

@LarsDenBakker

CommonJS‑style resolve includes the package exports and imports fields, along with the self‑import when exports is specified:

@LarsDenBakker
Copy link
Contributor Author

@ExE-Boss that's correct, but esm imports have different resolve logic such as no magic index.js handling and requiring file extensions.

@LarsDenBakker LarsDenBakker force-pushed the feat/node-resolve-improvements branch 2 times, most recently from cf92a06 to 816786d Compare December 12, 2020 08:37
@LarsDenBakker
Copy link
Contributor Author

@shellscape I see that circle CI is failing on a security warning, I suspect that warning is there on master as well.

@ljharb
Copy link

ljharb commented Dec 13, 2020

@LarsDenBakker resolve intends to implement every node resolution algorithm. The first step is the linked PR, which will lead to full "exports" support for CJS; the next step will be to release a new async-only API which provides ESM resolution.

@shellscape
Copy link
Collaborator

@LarsDenBakker please pull from upstream/master to fix the audit error in the analysis step. (note that you can create branches directly on this repo now)

@LarsDenBakker
Copy link
Contributor Author

@shellscape done, thanks! My local repo is still configured to use my remote, will update that.

@shellscape
Copy link
Collaborator

For those participating; we've had some discussion internally about whether to wait for browserify/resolve to get that PR through, or deliver some value to the userbase immediately and have sided with value for the userbase immediately. We'll revisit these changes when resolve updates, but will merge and release for now.

@shellscape shellscape merged commit 0e4f014 into rollup:master Dec 14, 2020
@ljharb
Copy link

ljharb commented Dec 15, 2020

@shellscape please do follow that PR; when it lands and is released hopefully you'll be able to refactor to use resolve's implementation.

@lukastaegert
Copy link
Member

I think we should do definitely that. While I very welcome delivering this now for Rollup, your/browserify's package has always been an important centerpiece of this plugin and allowed us to have superior performance due to very intelligent directory-lookup caching possibilities.

If we can deliver the same functionality we implemented now using resolve, possibly even with better caching, then I think this would be a win for everyone. There was already one complaint with regard to the performance impact of this feature.

@LarsDenBakker
Copy link
Contributor Author

@shellscape @lukastaegert is it possible to cut a release of this?

@pi0
Copy link

pi0 commented Jan 15, 2021

v11.1.0 is out with fix thanks @guybedford <3

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.

Unresolved dependencies when "exports" field property's value is array
10 participants