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(node-resolve): pass on isEntry flag and custom options #1016

Merged
merged 1 commit into from
Oct 19, 2021

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Oct 8, 2021

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

Event though I raised the rollup version used in tests, the plugin will work nicely with older Rollup versions that do not support the isEntry flag. In this case, isEntry: undefined will be passed on as an option to this.resolve, which in turn will be ignored when the flag is not supported.

List any relevant issue numbers:
rollup/rollup#4230

Description

This makes sure the plugin correctly passes on the isEntry flag introduced in recent Rollup versions to the this.resolve context function as well as the custom option. This will allow plugins to rely on these to e.g. resolve entry points to proxy files or do any other conditional resolving.

As noted above, this is purely a bugfix and should have no adverse effect when used with older Rollup versions.

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.

Lgtm. Just spotted a typo

packages/node-resolve/test/test.js Outdated Show resolved Hide resolved
@lukastaegert lukastaegert force-pushed the fix/node-resolve-isentry branch 2 times, most recently from 3ff83fc to 27aa4e5 Compare October 8, 2021 08:40
@lukastaegert
Copy link
Member Author

Thanks. Also fixed the package file so that only the devDependencies version of Rollup changes, but not the peerDependencies version.

@lukastaegert lukastaegert marked this pull request as draft October 8, 2021 09:51
@lukastaegert
Copy link
Member Author

Switching to "draft" as after some thought, I think I should also forward the "custom" option to "this.resolve". Will convert it back once I added this.

@lukastaegert lukastaegert marked this pull request as ready for review October 8, 2021 11:06
@lukastaegert lukastaegert changed the title fix(node-resolve): pass on isEntry flag fix(node-resolve): pass on isEntry flag and custom options Oct 8, 2021
@lukastaegert
Copy link
Member Author

Done and again ready for review.

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.

👍

@shellscape shellscape merged commit 12e1fee into master Oct 19, 2021
@shellscape shellscape deleted the fix/node-resolve-isentry branch October 19, 2021 13:45
@@ -261,9 +260,13 @@ export function nodeResolve(opts = {}) {
importer = undefined;
}

const resolved = await doResolveId(this, importee, importer, opts);
const resolved = await doResolveId(this, importee, importer, resolveOptions.custom);

Choose a reason for hiding this comment

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

This commit broke plugin-node-resolve, see: #1023

Copy link
Member

Choose a reason for hiding this comment

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

opened #1029

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.

4 participants