Skip to content

Don't rely on Array.prototype.map#31

Merged
kpdecker merged 2 commits intokpdecker:masterfrom
papandreou:map
Sep 9, 2014
Merged

Don't rely on Array.prototype.map#31
kpdecker merged 2 commits intokpdecker:masterfrom
papandreou:map

Conversation

@papandreou
Copy link
Contributor

Seems like this is the only thing that makes https://github.com/visionmedia/mocha not work in older browsers.

@kpdecker
Copy link
Owner

kpdecker commented Sep 2, 2014

What browsers are of concern here?

@papandreou
Copy link
Contributor Author

IE8 and below. Still relevant in some settings, especially since your lib is used by test runners and assertion libraries.

diff.js Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

What is this in operation doing? Seems like it's just overhead for an iterator that is walking length.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I didn't think about that, I just copied the function from somewhere else. I think it originates from https://github.com/LearnBoost/expect.js/blame/master/index.js#L812-L824

It seems like the reason for that check is sparse arrays. If I comment out the check and the use of Array.prototype.map I get:

console.log(map(new Array(5), function (item) {return 10;})); // [10, 10, 10, 10, 10]

as opposed to: [ , , , , ] when the check is there.

Copy link
Owner

Choose a reason for hiding this comment

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

I'd remove it as none of the uses here are spare arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, done.

@kpdecker
Copy link
Owner

kpdecker commented Sep 9, 2014

Looks like this has a merge conflict with some other refactoring work that went in. If you can rebase this I'll merge. Otherwise I'll try to rebase but it might take a little bit of time.

@papandreou
Copy link
Contributor Author

Rebased.

@kpdecker
Copy link
Owner

kpdecker commented Sep 9, 2014

Awesome! Thank you.

kpdecker added a commit that referenced this pull request Sep 9, 2014
Don't rely on Array.prototype.map
@kpdecker kpdecker merged commit c178175 into kpdecker:master Sep 9, 2014
@papandreou
Copy link
Contributor Author

Thanks! How about a new npm release soon?

@kpdecker
Copy link
Owner

Released in 1.1.0

@kpdecker
Copy link
Owner

Sorry for the delay, I was hoping to try to resolve some of the other issues in a larger release but I opted to get this out and get it out of my TODO list (as I was ignoring the item, which didn't help).

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.

2 participants