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

Make sure stringIteratorExists is checking for iterator function on String prototype #93

Merged
merged 1 commit into from Apr 11, 2017

Conversation

JakeChampion
Copy link
Contributor

@JakeChampion JakeChampion commented Apr 10, 2017

Currently it is checking for it on the Array prototype, which is incorrect as a browser could have the Array iterators and not have the String iterators.

@keithamus
Copy link
Member

keithamus commented Apr 10, 2017

Good catch! Thanks @JakeChampion! Also thanks for your first contribution to type-detect! It would be useful to have a test to ensure no regressions. Any thoughts about how we could test that? Is there a specific browser you saw this in?

@JakeChampion
Copy link
Contributor Author

I found the issue in PhantomJS 1.9.2 whilst using https://polyfill.io to polyfill missing features. It seems https://polyfill.io didn't have a String iterator polyfill, that will be fixed shortly (polyfillpolyfill/polyfill-service#1165).

I'm not too sure how to test this except to do what I did, open PhantomJS 1.9.2 using Karma and adding this file to load in the browser - https://polyfill.io/v2/polyfill.js?features=default,es5,es6,es7,es2015,es2016,es2017,NodeList.prototype.@@iterator,Array.prototype.@@iterator&flags=gated

@keithamus
Copy link
Member

Let's just get this in. It's an obvious mistake that should be fixed. I'm not really sure how to test for it, so we can just merge this provided another @chaijs/type-detect member approves.

@JakeChampion
Copy link
Contributor Author

@lucasfcosta Does the thumbs up count as an approved review?

@shvaikalesh
Copy link
Contributor

I believe it is: LGTM. Thanks for PR, @JakeChampion.

@lucasfcosta
Copy link
Member

@JakeChampion yes it does! I had typed a comment approving this change but I forgot to click the comment button. Sorry! 😅

@lucasfcosta lucasfcosta merged commit 71bcdac into chaijs:master Apr 11, 2017
@JakeChampion JakeChampion deleted the patch-1 branch April 11, 2017 12:48
@JakeChampion
Copy link
Contributor Author

Thanks! May I ask how long it will be until this is released onto npmjs.com?

@keithamus
Copy link
Member

Ahh. It would have auto-released as 4.0.1 if the commit message was formatted to conventional commit message standards. That's our bad, I'll try to get it released tonight.

@chaijs/type-detect we should look to providing some checks to ensure commits going into master are conventional commit messages - for now let's eyeball, and "Squash and merge". We can look at tooling around this soon.

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.

None yet

4 participants