Fixed a bug where core-js was modifying native collection iterator prototypes#261
Fixed a bug where core-js was modifying native collection iterator prototypes#261aickin wants to merge 2 commits intozloirock:masterfrom
Conversation
…ototypes incorrectly; it should have been modifying the prototype of the prototype.
|
You are right, it's incorrect behaviour, but initially it was added intentionally. Early iterators implementations haven't |
…riginal use of the code. Also added a big comment for folks who come later to this chunk and have similar misunderstandings as I did.
|
That is extremely helpful feedback, @zloirock! I could tell that there was some sort of backwards compatibility reason, but it wasn't entirely clear to me. With this feedback, I modified the PR to hopefully still patch old browsers but not interfere with newer, spec-compliant browsers in 7d54d72. Instead of checking to see if the iterator's prototype has I think this should maintain existing core-js behavior in browsers that don't have %IteratorPrototype% while not modifying the prototype chain in spec-compliant browsers. I also added a big wordy comment explaining this for folks who may come to the code and misunderstand it in the way that I did. That may conflict with your comment style, and I'll happily modify it if you wish. Just let me know. Thanks again for your work on such a critical piece of web infrastructure! |
|
Hey there! I'm wondering if I can respectfully bump this PR a little. I think the new implementation addresses your concerns while allowing newer browsers to have the correct prototype chain. Any feedback on the PR? Thanks so much for your time, and again: thanks for all your work! |
|
Fixed in |
First off, thanks for all the hard work that has gone into
core-js!I ran into a fairly obscure bug in
core-js's handling of collection iterator prototype chains while running compat-table tests on various platforms. I found thatcore-jswas causing collection iterator prototype chain tests to fail on platforms that already had native collection iterator support. The tests that were failing are for iterators on Set, Map, Array, and String, and they all look basically like this:As this test shows, the iterator should not have a
Symbol.iteratorown property, and neither should the iterator's prototype. The iterator's prototype's prototype, however, should have aSymbol.iteratorproperty, and that method should return the iterator. These tests pass in all modern browsers without any shims (Chrome, Firefox, Safari). They also pass whencore-jsis used in a browser that has no iterator support. These tests fail, however, when used in modern browsers in concert withcore-js.It looks to me like there is a method in
_iter-define.jsthat attempts to patch the iterator prototype chain for native iterator implementations only, but the implementation puts aSymbol.iteratormethod on the iterator's prototype rather than the iterator's prototype's prototype. This is what causes the test failures.This PR attempts to fix this by patching the native iterator's prototype's prototype instead. I added tests for Set, Map, Array, and String, although those tests didn't fail with the pre-existing code. I think this because
npm run testruns the test suite in Phantom, which doesn't have native iterator support. As I said above, this bug only shows up in browsers that have native iterator support.Apologies for any breaches of code style and such; I've never used LiveScript before and did not feel super confident about my changes there. Feedback is welcome, and thanks again for your work!