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

IE 11 not supported #1951

Closed
eps1lon opened this issue Nov 19, 2018 · 16 comments
Closed

IE 11 not supported #1951

eps1lon opened this issue Nov 19, 2018 · 16 comments

Comments

@eps1lon
Copy link

eps1lon commented Nov 19, 2018

Describe the bug
IE 11 environment is not supported with regards to syntax and methods. For example sinon has transitive dependencies to has-flag which is an arrow function and uses String.prototype.startsWith. Missing syntax support wouldn't be as bad since it can be included in babel-loader but adding polyfills is kind of rough since this would also mean that it wouldn't warn us if we use startsWith. Maybe there is also an option to only include polyfills for certain node_modules but both points can't really be attributed to IE 11 support.

To Reproduce
Steps to reproduce the behavior:

  1. Build sinon-esm.js
  2. search for hasFlag which is an arrow function and uses startsWith

Expected behavior
Support for IE 11 out of the box.

Screenshots
None

Context (please complete the following information):
We're trying run unit tests for our react components in IE 11.

  • Library version: 7.1.1
  • Environment: IE 11
  • Example URL: none
  • Other libraries you are using: babel, enzyme, karma, webpack

I guess enzyme will be another bottleneck which requires airbnb-browser-shims anyway and I couldn't find a solution to provide polyfills only to certain dependencies.

@fatso83
Copy link
Contributor

fatso83 commented Nov 19, 2018

We are running Sinon in IE11 in our own browser tests. IE11 does not support Ecmascript Modules, so it makes no sense to use the sinon-esm build.

Just use the Sinon.js bundle. Works everywhere.

@fatso83 fatso83 closed this as completed Nov 19, 2018
@oliviertassinari
Copy link

oliviertassinari commented Nov 19, 2018

@fatso83 The issue we are hitting with the esm version isn't with Ecmascript Modules, it's with the ES6 syntax, like arrow functions that are not transpiled down to ES5.

@oliviertassinari
Copy link

oliviertassinari commented Nov 19, 2018

@fatso83 I don't understand, there is always a way to make it work. My point is with the community convention. As far as I have seen this in the wild, library authors are providing esm version of their library so people can leverage tree shaking. It's not about targeting evergreen browsers (ES6+).

@fatso83
Copy link
Contributor

fatso83 commented Nov 19, 2018

@oliviertassinari, I get that, but if you are consuming an Ecmascript module you shouldn't expect it to be ES5 to begin with (as it's not). You will require some sort of transpilation step in your build pipeline if you build a bundle from ES modules, as this is not valid ES5, but it IS a ES module:
export { _createFakeServerWithClock as createFakeServerWithClock };

I suspect that your webpack config excludes node_modules from undergoing Babel transpilation, so you either need to selectively include Sinon or just import the non-esm bundle instead.

Chances are I just had the same issue with the Material UI React lib (what are the chances, Olivier!), so you could employ my fix: https://stackoverflow.com/q/53154986/200987

@fatso83
Copy link
Contributor

fatso83 commented Nov 19, 2018

But I do agree that it would be wise to make the most common thing as easy as possible, so if a simple config change can produce a valid ESM that will not cause problems in most builds, we will accept that PR.

@oliviertassinari
Copy link

oliviertassinari commented Nov 19, 2018

you shouldn't expect it to be ES5 to begin with

What should I expect it to be, can it have ES2018 syntax?

Chances are I just had the same issue with the Material UI React lib (what are the chances, Olivier!)

I would love to know more about it. Right now the target with Material-UI is to have 3 outputs.

  • One for ES5
  • One for ESM (ES5 + es modules)
  • One for evergreen browsers (es) basically using all the official syntax, so ES2018 this year, ES2019 next year, etc.

Thanks for the link, @eps1lon is letting Babel transpile Sinon to solve the problem on our side.

@eps1lon
Copy link
Author

eps1lon commented Nov 20, 2018

I used the module build because it's easier to investigate since everything is in one place. This issue stays the same if I tell webpack to use the main entry and therefore commonJS modules.

The modules used are of no concern for the generated bundle.

@fatso83 If I have to transpile node_modules anyway then you can't state that it supports IE 11. That's not how this works. Please consider reopening this or at direct me to a working configuration that uses sinon in IE 11.

@mantoni
Copy link
Member

mantoni commented Nov 20, 2018

This discussion doesn't seem very fruitful. I'll try to explain the Sinon.JS teams view:

To begin with, the statement from our latest API documentation page is what we feel bound to. It sais

Sinon v7.1.1 is written as ES5.1 and requires no transpiler or polyfills to run in the runtimes listed below.

The listed runtimes are

  • Firefox 45
  • Chrome 48
  • Internet Explorer 11
  • Edge 14
  • Safari 9
  • Node.js LTS versions

Obviously, these statements are made regarding the standard Sinon.JS distribution, as it is downloadable from our web page and distributed via npm. The links provided above show the successful runs of our test suite on IE 11, among other browsers.

We provide an ESM bundle for environments where ESM is supported, to make it possible to consume Sinon in these environments. ESM is not supported by IE 11.

Now, what you're asking for is to make an ESM version of Sinon for a target environment that doesn't support ESM. This is out of the scope of this project.

What @fatso83 was trying to explain is, that if you have a need like this, you're most likely transpiling your code anyway, since IE 11 does not support ESM out of the box. We're therefore asking you to take the regular Sinon build, which does support IE 11, and wrap it in an ESM bundle yourself, just as you're most likely doing it with other resources.

We will not re-open this, as it's not an issue with Sinon. I hope you understand.

@eps1lon
Copy link
Author

eps1lon commented Nov 20, 2018

@mantoni Please read my comment again and consider that module and main are only concerned about the module system not other syntax or language features.

@mroderick
Copy link
Member

@eps1lon @oliviertassinari I am not against the ESM version of Sinon being ESM + ES5.1, if that solves problems for people. Would you care to submit a pull request to scratch your itch?

@eps1lon
Copy link
Author

eps1lon commented Nov 20, 2018

Just to be perfectly clear the commonJS version isn't ES5.1 either which should be showcased with https://github.com/eps1lon/sinon-ie11

The issue is that you have a transitive dependency to the has-flag package via the main entry which uses fat arrow syntax as well as String.prototype.startsWith.

$ npm ls has-flag
sinon-ie-11@1.0.0 C:\Development\src\js\sinon-ie-11
`-- sinon@7.1.1
  `-- supports-color@5.5.0
    `-- has-flag@3.0.0

This is fixable by setting mainFields to ['browser', 'main', 'module'] but that's not what I usually have to tell webpack if I'm building for the browser and since webpack defaults to ['browser', 'module', 'main'] I would expect libraries to consider this as well.

I'm going to check if I can fix your esm build with a similar approach but I have to investigate if rollup allows mainFields configuration.

Edit:
This is held back by rollup/rollup-plugin-node-resolve#182 so it's only solvable in userland for now. I'm also not even sure that you would want that since you apparently only consider your code when talking about compatibility.

@mantoni
Copy link
Member

mantoni commented Nov 20, 2018

The CommonJS version is ES5.1. You can search through the pkg/sinon.js sources and you will neither find arrow functions nor any use of startsWith. That is because the transitive dependency you're referring to is not bundled because supports-color has a a browser version which simply states that color is not supported.

Again, if you're using Sinon in a browser, use the browser package. If you want the browser package within an ESM, you're invited to send a PR to produce a new ESM + ES5.1 bundle for Sinon. The current bundle does not need fixing. It's not broken.

@eps1lon
Copy link
Author

eps1lon commented Nov 20, 2018

@mantoni I wish I never mentioned the esm build. It has nothing to do with the issue. It's however troubling that you only consider your code as part of the package and not dependencies. I hope you don't have a similar attitude towards security issues in dependencies.

@mantoni
Copy link
Member

mantoni commented Nov 20, 2018

We do consider dependencies as part of our library. We also take security very seriously and watch security alerts on the repository and npm audit closely.

Let me try again, without using the word esm: I understand that you're loading the Sinon sources into IE 11. You're obviously not loading the browser version, because the browser version does not have the issues you're referring to. You should be loading the browser version if you want to use Sinon in a browser. Currently, only the pkg/sinon.js file is built to run in browsers. Apparently you're trying to use Sinon in an unsupported way. Since this is open source, you're invited to make it work by sending a pull request.

@fatso83
Copy link
Contributor

fatso83 commented Jul 14, 2019

People in this thread might be interested in knowing that @Feiyang1 implemented a fix for this by adding a browser field in package.json. See #2060 (comment)

There should be no additional configuration needed for Webpack/Babel builds after this has shipped.

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

No branches or pull requests

5 participants