Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(node-resolve): fix "browser" field mappings. #786

Closed
wants to merge 2 commits into from

Conversation

DorianBlues
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: #472, #540

Description

When browser option of node-resolve is true, the package.json browser filed should override the export map result.
But I found that node-resolve not handled it correctly when use commonjs to require some pacakage. (e.g. nanoid)

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.

I think we should modify this so that the "browser" exports should take precedence over the base-level "browser" field, as otherwise it will be too confusing when each will apply.

@DorianBlues
Copy link
Contributor Author

I found that the "browser" exports of package nanoid has optimization conditions: development and production which now are supported by webpack. Should we change the type of "browser" option of node-resolve to string so that we can support it.

@guybedford
Copy link
Contributor

@DorianBlues right, you can enable these via "conditions": ["development"] etc also being passed to the plugin.

It might also make sense for RollupJS to default to a development or production condition, for cases where these are treated as mutually exclusive by packages like nanoid. Note these are now all endorsed by Node.js fwiw (https://nodejs.org/dist/latest-v15.x/docs/api/packages.html#packages_conditions_definitions). //cc @LarsDenBakker

@shellscape
Copy link
Collaborator

@DorianBlues please give this PR some attention. When PRs have no activity for 30 days, we typically close them as abandoned unless the author responds.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants