Skip to content

add browser field in package.json#2060

Merged
mantoni merged 1 commit intosinonjs:masterfrom
Feiyang1:browser-field
Jul 13, 2019
Merged

add browser field in package.json#2060
mantoni merged 1 commit intosinonjs:masterfrom
Feiyang1:browser-field

Conversation

@Feiyang1
Copy link
Copy Markdown
Contributor

Purpose (TL;DR) - mandatory

It makes bundlers(webpack, rollup) to pick up the es5 version by default when targeting browsers. It solves issues like #1951

How to verify - mandatory

  1. Check out this branch
  2. npm install
  3. use webpack to bundle tests without compiling sinon
  4. run tests in non-es6 ready browsers, e.g. IE11
  5. tests should run successfully

@coveralls
Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 2926

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.186%

Totals Coverage Status
Change from base Build 2923: 0.0%
Covered Lines: 1660
Relevant Lines: 1729

💛 - Coveralls

@mantoni
Copy link
Copy Markdown
Member

mantoni commented Jul 13, 2019

The browser field can be used to override the “main” for browsers. This change point browsers to the same file so I don’t think it makes a difference.

@Feiyang1
Copy link
Copy Markdown
Contributor Author

Feiyang1 commented Jul 13, 2019

webpack checks field in package.json using ['browser', 'module', 'main'], in that order, when targeting browsers. Without the browser field, module field will be used which is the esm + es6 version, so issues like #1951 will happen.

People can change webpack configuration to make it work, but it's extra burden for them and sometimes is not obvious to people who don't know webpack well. Having browser field defined will enable sinon to work in any browser environment without additional configuration.

@mantoni
Copy link
Copy Markdown
Member

mantoni commented Jul 13, 2019

Thank you for clarifying. Now I understand.

@mantoni mantoni merged commit fe7347e into sinonjs:master Jul 13, 2019
@fatso83 fatso83 mentioned this pull request Jul 14, 2019
@fatso83
Copy link
Copy Markdown
Contributor

fatso83 commented Feb 23, 2020

@Feiyang1 Why did we not choose the prebuilt file, ./pkg/sinon.js with this change? That is guaranteed to work, whereas using the lib/sinon.js makes producing a working Sinon build for tests up to the ones bundling sinon into their builds.

I don't think we would have issues like #2218 (comment) if we used build package, but I might be forgetting something ...

@mantoni
Copy link
Copy Markdown
Member

mantoni commented Feb 23, 2020

Well, when I'm running my test cases in a browser, I usually browserify my tests along with Sinon. If the browser field would point to a pre-bundled version this would have unwanted effects. For example, if a library is used that is also used by Sinon, it would be bundled multiple times, causing issues with instanceof and Object.isPrototypeOf checks.

@mantoni
Copy link
Copy Markdown
Member

mantoni commented Feb 23, 2020

The way I see it, the pre-bundled Sinon packages are for <script> tags. The browser field is explicitly there for bundlers and therefore shouldn't point to a pre-bundled version.

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