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

Improve support for Internet Explorer #77

Merged
merged 3 commits into from Mar 9, 2017

Conversation

Projects
None yet
3 participants
@c-w
Copy link
Contributor

commented Mar 8, 2017

This pull request fixes two bugs with cloning Maps and Sets on IE:

  • IE requires the type argument to instanceof to be defined.
  • IE does not implement Map.keys() and Set.keys().

These bugs currently cause the "clone a native Map" and "clone a native Set" tests in test.html to fail on Internet Explorer:

image

After applying the changes in this pull request, all the test-cases in test.html pass on Internet Explorer, Chrome and Firefox.

c-w added some commits Mar 8, 2017

Fix instanceof crash on IE
According to the MSDN documentation [1], in the Javascript engine of
Internet Explorer, the class passed into the instanceof method must be
defined or the instanceof method may crash.

This commit introduces a wrapper around instanceof that makes sure that we
never call `something instanceof null` or `something instanceof undefined`
thus avoiding this source of crashes.

[1] https://msdn.microsoft.com/en-us/library/zh0zb36z(v=vs.94).aspx#Parameters
Fix map/set clone crash on IE
Internet Explorer's implementation of the Map and Set types does not
provide a `myset.keys()` method which means that cloning those types fails
when running on IE.

This commit replaces the calls to `keys` with `forEach` which is supported
in Chrome, Firefox and IE [1, 2].

[1] https://msdn.microsoft.com/en-us/library/dn280909(v=vs.94).aspx
[2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/forEach
@rictic

rictic approved these changes Mar 9, 2017

Copy link
Contributor

left a comment

As author of the code in question this looks good to me.

@pvorb

This comment has been minimized.

Copy link
Owner

commented Mar 9, 2017

Yes, I also had a look and everything looks good to me.

@c-w Thanks for testing this on IE and fixing the issue right away. If you'd like to be mentioned in the package.json, feel free to add yourself to the list of contributors and push again on your branch.

@c-w

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2017

@rictic Thanks for the signoff.
@pvorb Thanks for the suggestion. Done.

@pvorb pvorb merged commit e3f252d into pvorb:master Mar 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pvorb

This comment has been minimized.

Copy link
Owner

commented Mar 9, 2017

BTW, v2.1.1 is on npm

@c-w

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2017

Thanks for the quick merge and version bump. I encountered these bugs while working with QuillJS so maybe we'll enable them to support older versions of IE now too.

jhchen added a commit to quilljs/quill that referenced this pull request Mar 9, 2017

Update clone dependency to >=2.1.1 <2.2 (#1359)
In version 2.1.1, CloneJS improved support for Internet Explorer. See
this pull request for details: pvorb/clone#77
@pvorb

This comment has been minimized.

Copy link
Owner

commented Mar 9, 2017

That project looks promising, didn't know about it. Nice to see you can make use of this little library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.