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

Provided browser field in package.json #875

Merged
merged 1 commit into from Nov 24, 2016

Conversation

Projects
None yet
4 participants
@bdadam
Contributor

bdadam commented Nov 23, 2016

Some tools use the browser field in the package.json to use an alternate main file. This is e.g. the case for rollup-plugin-node-resolve (https://github.com/rollup/rollup-plugin-node-resolve).
Specs: https://github.com/defunctzombie/package-browser-field-spec#alternate-main---basic

Provided browser field in package.json
Some tools use the browser field in the package.json to use an alternate main file. This is e.g. the case for rollup-plugin-node-resolve (https://github.com/rollup/rollup-plugin-node-resolve).
Specs: https://github.com/defunctzombie/package-browser-field-spec#alternate-main---basic
@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus
Member

keithamus commented Nov 23, 2016

LGTM! /cc @chaijs/chai

@shvaikalesh

This comment has been minimized.

Show comment
Hide comment
@shvaikalesh

shvaikalesh Nov 23, 2016

Member

Hello @bdadam, thanks for the PR!

Personally, I prefer not to have bundled dist in the repo. It is extra file and extra commits. There are tools like https://wzrd.in/ for users that have no building process or js cdns. For those who do have, for example, Rollup or other modern tree-shaking stuff, Browserify bundle will add extra KBs to the package.

Member

shvaikalesh commented Nov 23, 2016

Hello @bdadam, thanks for the PR!

Personally, I prefer not to have bundled dist in the repo. It is extra file and extra commits. There are tools like https://wzrd.in/ for users that have no building process or js cdns. For those who do have, for example, Rollup or other modern tree-shaking stuff, Browserify bundle will add extra KBs to the package.

@bdadam

This comment has been minimized.

Show comment
Hide comment
@bdadam

bdadam Nov 23, 2016

Contributor

Hello @shvaikalesh, thanks for your response. It is not about providing bundled dist files or not (it seems to be the case anyways, see: https://github.com/chaijs/chai/blob/master/chai.js). It is about which main file to use for builds for the browser.

I have right now the case, that I would like to require('chai') in a file which is then bundled with rollup and used in karma tests in browsers. Currently the bundled file is unusable, because rollup cannot do anything with buffer. Browsers don't understand buffer either. But it works perfectly fine if I require('chai/chai.js'), because that file is supposed to be the entry point for browser builds. It is the main file in the bower.json as well.

If the package.json includes the browser field, than that is preferred over the main entry by the rollup-plugin-node-resolve.

Do you see any problem with using chai.js as the main entry?

Contributor

bdadam commented Nov 23, 2016

Hello @shvaikalesh, thanks for your response. It is not about providing bundled dist files or not (it seems to be the case anyways, see: https://github.com/chaijs/chai/blob/master/chai.js). It is about which main file to use for builds for the browser.

I have right now the case, that I would like to require('chai') in a file which is then bundled with rollup and used in karma tests in browsers. Currently the bundled file is unusable, because rollup cannot do anything with buffer. Browsers don't understand buffer either. But it works perfectly fine if I require('chai/chai.js'), because that file is supposed to be the entry point for browser builds. It is the main file in the bower.json as well.

If the package.json includes the browser field, than that is preferred over the main entry by the rollup-plugin-node-resolve.

Do you see any problem with using chai.js as the main entry?

@lucasfcosta

This comment has been minimized.

Show comment
Hide comment
@lucasfcosta

lucasfcosta Nov 23, 2016

Member

@bdadam Thanks for your PR! LGTM.
Also, your explanation seems pretty good and I see no reason not to add this to our package.json, since we're not adding any new file, just pointing to an existing one which might be useful for other people.
I'll leave this open for @shvaikalesh to merge anyway.

Member

lucasfcosta commented Nov 23, 2016

@bdadam Thanks for your PR! LGTM.
Also, your explanation seems pretty good and I see no reason not to add this to our package.json, since we're not adding any new file, just pointing to an existing one which might be useful for other people.
I'll leave this open for @shvaikalesh to merge anyway.

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Nov 24, 2016

Member

@shvaikalesh we do make heavy use of the browserified bundle, for example it is used by all of the karma plugins. We could push it out into a different package, but thats probably out of scope for the PR. This is really a small change just to link it - shall we move the discussion of strategies for browser bundling to a separate issue?

Member

keithamus commented Nov 24, 2016

@shvaikalesh we do make heavy use of the browserified bundle, for example it is used by all of the karma plugins. We could push it out into a different package, but thats probably out of scope for the PR. This is really a small change just to link it - shall we move the discussion of strategies for browser bundling to a separate issue?

@shvaikalesh

This comment has been minimized.

Show comment
Hide comment
@shvaikalesh

shvaikalesh Nov 24, 2016

Member

@keithamus Agreed, (not) having bundle in repo deserves a discussion in another issue.

Member

shvaikalesh commented Nov 24, 2016

@keithamus Agreed, (not) having bundle in repo deserves a discussion in another issue.

@shvaikalesh shvaikalesh merged commit b7ff144 into chaijs:master Nov 24, 2016

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bdadam

This comment has been minimized.

Show comment
Hide comment
@bdadam

bdadam Nov 24, 2016

Contributor

Thanks everyone for merging the PR.

Contributor

bdadam commented Nov 24, 2016

Thanks everyone for merging the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment