Skip to content

Conversation

@lukastaegert
Copy link
Member

Rollup Plugin Name: commonjs

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

Description

cc @LarsDenBakker This is the necessary change to move forward with #540. When the commonjs plugin is resolving imports via this.resolve, this will pass on a custom options object of the form

{
  'node-resolve': { isRequire: true }
}

This can then be picked up by the node-resolve plugin via #540 to decide if whether it should add the import or the require condition.

I decided against a more general format here not because I do not think it would be useful but because it was the easiest to just solve the problem at hand and also avoid the need for a validation (i.e. the node-resolve logic would implement the mutual exclusivity of the import and require conditions). However if we want to have general conditions, we would need to add another option that would possibly interact with this option, so this may be a bad idea after all. Feedback welcome.

Once we agreed upon this PR, this can be merged at any time without this being a breaking change. Because even though it relies on new or even not-yet implemented features in Rollup and the node-resolve plugin, it will work gracefully with older versions as those will just ignore the custom parameter. And maybe this should actually be merged first so that when #540 is released, it can be fully functional from the start.

Copy link
Contributor

@LarsDenBakker LarsDenBakker left a comment

Choose a reason for hiding this comment

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

Looks good to me

@shellscape shellscape changed the title Pass type of import to node-resolve feat(commonjs): pass type of import to node-resolve Oct 17, 2020
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.

Great stuff!

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.

5 participants