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

Fix set iterableToString test on IE11 #1230

Merged
merged 1 commit into from
Jan 2, 2017

Conversation

lucasfcosta
Copy link
Member

@lucasfcosta lucasfcosta commented Jan 2, 2017

Purpose (TL;DR)

This fixes the failing tests on IE11 introduced in #1215 .

Background

Our implementation of typeOf was too simple and didn't handle older implementations of Maps and Sets.

Even with the if clauses guarding the Map and Set related tests they would fail, because IE11 did implement them, however, IE11's implementation wasn't following the ECMA standard. This caused our type detection to fail without recognizing these object's types correctly.

Also, IE11's Set constructor was a bit weird and didn't accept an array of elements as argument, so I had to use multiple .add calls to add elements to a Set.

Solution

I added type-detect to solve the typeOf related problem.

I'm not sure everyone agrees on that. So, if you don't want that please let me know and I'll dedicate more time to rewrite typeOf and make it compatible with older browsers implementations of Maps and Sets. However, this library is maintained by everyone at Chai and other awesome contributors and it's pretty stable and has good performance.

IMO, having less code to maintain is always good and maybe we can join efforts on that library making it better for everyone.

I have also changed the Set constructor on a test in order to use an IE11 compliant way of creating Sets.

How to verify - mandatory

  1. Check out this branch (see github instructions below)
  2. npm install
  3. npm run test

Let me know if you disagree with anything and I'll happily fix it.

PS.: Sorry for breaking your build! 😅

@coveralls
Copy link

coveralls commented Jan 2, 2017

Coverage Status

Coverage decreased (-0.04%) to 95.407% when pulling a1b3693 on lucasfcosta:fix-set-iterableToString-ie11 into 3500d60 on sinonjs:master.

Copy link
Contributor

@fatso83 fatso83 left a comment

Choose a reason for hiding this comment

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

I had no idea that the typeOf code was that simple. I have no objections to this, as it seems like a very minor thing.

@fatso83
Copy link
Contributor

fatso83 commented Jan 2, 2017

Great PR descriptons btw! Making the life of maintainers much easier.

@fatso83 fatso83 merged commit a6d8e14 into sinonjs:master Jan 2, 2017
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