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(rollup_bundle): add resolve({..., browser:true}) #441

Closed

Conversation

IgorMinar
Copy link
Contributor

This is useful for bundling zone.js and other npm packages that contains
the browser field in the package.json

This is useful for bundling zone.js and other npm packages that contains
the browser field in the package.json
@gregmagolan
Copy link
Collaborator

gregmagolan commented Dec 1, 2018

Just a note that the browser field will take precedence over module which may be undesirable in some cases since browser code will not be ES6 typically. https://github.com/rollup/rollup-plugin-node-resolve/blob/master/src/index.js#L140

Maybe this should option should be turned on with an attribute?

Also, this will hopefully land soon as well which will support es2015 needed to bundle rxjs: rollup/rollup-plugin-node-resolve#182 which means we could expose a mainFields attribute in rollup_bundle as well.

@alexeagle
Copy link
Collaborator

This PR ought to include a test of some kind. Also, could you explain why this change? (is there some accompanying change in Angular? what's wrong with how we load zone.js now?)

Per @gregmagolan comment, how can we even know what is the right set of attributes here? Users need browser field to resolve if and only if one of their packages uses it, but otherwise it's undesirable?

@alexeagle alexeagle self-requested a review December 3, 2018 15:36
@filipesilva
Copy link
Collaborator

rollup/rollup-plugin-node-resolve#182 is now merged into Rollup.

@alexeagle
Copy link
Collaborator

Not planning to add features to old rollup-bundle rule, will be deleted by 1.0

@alexeagle alexeagle closed this Sep 20, 2019
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.

None yet

5 participants