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
Merged

Improve support for Internet Explorer #77

merged 3 commits into from
Mar 9, 2017

Conversation

c-w
Copy link
Contributor

@c-w c-w 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 2 commits March 8, 2017 13:38
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
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
Copy link
Contributor

@rictic rictic left a comment

Choose a reason for hiding this comment

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

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

@pvorb
Copy link
Owner

pvorb 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
Copy link
Contributor Author

c-w 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
@pvorb
Copy link
Owner

pvorb commented Mar 9, 2017

BTW, v2.1.1 is on npm

@c-w
Copy link
Contributor Author

c-w 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 pushed a commit to slab/quill that referenced this pull request Mar 9, 2017
In version 2.1.1, CloneJS improved support for Internet Explorer. See
this pull request for details: pvorb/clone#77
@pvorb
Copy link
Owner

pvorb 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants