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 compatibility when Symbol is polyfilled #47

Merged
merged 3 commits into from
Apr 30, 2020

Conversation

wbinnssmith
Copy link
Contributor

@wbinnssmith wbinnssmith commented Apr 29, 2020

Resolves #40.

Currently, with a Symbol polyfill present in IE 11, requiring util/types throws (see #40 for more detail). With Symbol and Symbol.toStringTag available, IE 11 then attempts to use it on the Uint8Array prototype, where it does not exist.

This:

  • Adds another set of tests with polyfills present in the environment. I'm happy to restrict this to just the Symbol polyfill as that should be all that's necessary to reproduce the issue. This additional set of tests was also added to the travis configuration to run in Sauce Labs.
  • This follows @ljharb's advice in In IE 11, SCRIPT5007: Unable to get property 'get' of undefined or null reference #40 and uses is-typed-array and which-typed-array to determine if values are typed arrays and which type they are.

Ideally this is published in a patch release or https://github.com/browserify/commonjs-assert is updated to use it, as folks seem to run into this through their use of assert (though it's certainly possible to use util/types directly).

cc @mischnic @devongovett @padmaia @stacylondon

Test Plan: Ran both sets of tests (with and without polyfills) in IE 11 with success.

@wbinnssmith
Copy link
Contributor Author

wbinnssmith commented Apr 29, 2020

I'm not sure if the Sauce Labs tests are running correctly. I forgot to update this:

https://github.com/browserify/node-util/compare/0fa099ed3b002be39c251dabcba699608fee526e..4068972016662efd87da60ffabae1d03b96d4955

Using a non-existent script should result in a non-zero exit code, but Travis seems to have passed: https://travis-ci.org/github/browserify/node-util/builds/681196592

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a clean semver-patch that makes these checks go from "not particularly robust" to "as unbreakable as i know to be possible" :-)

i'm obviously a bit biased, so this should get more eyes before landing.

@lukechilds
Copy link
Member

This looks good to me! I wish I'd known about which-typed-array/is-typed-array before writing this 😅

Copy link
Member

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saucelabs tests don't run on pull requests—pushed a branch to trigger a full run here: https://travis-ci.org/github/browserify/node-util/jobs/681393138

Will merge once that comes up green. Thanks for working on this!

@goto-bus-stop goto-bus-stop merged commit cb72df7 into browserify:master Apr 30, 2020
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.

In IE 11, SCRIPT5007: Unable to get property 'get' of undefined or null reference
4 participants