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

Resolve yield * issues for Firefox < 45 #278

Merged
merged 2 commits into from
Apr 28, 2017

Conversation

rayd
Copy link
Contributor

@rayd rayd commented Feb 15, 2017

Attempt to address #274. We're basically checking whether or not the NativeIteratorPrototype's next function is the bad LegacyIteratorNext function. If it is, then we choose to use the polyfill. If it's not and we would have otherwise been using the native iterator, then we use the native iterator implementation.

Running the browser tests in test/index.html confirms that things work in Firefox < 45, in Firefox > 45 and in Chrome. Thoughts?

@intentionally-left-nil
Copy link

intentionally-left-nil commented Feb 15, 2017

The approach seems reasonable to me. Another way of doing the detection instead of checking the name would be to make a simple iterator, call next() and see if it returns done: true. If it doesn't, then it's the bad one.

I personally think either approach is reasonable, since this is a specific-legacy-browser workaround. (meaning that LegacyIteratorNext isn't going to change because it only affects certain old and static builds)

Thanks for providing a fix!

// This environment has a native %IteratorPrototype%; use it instead of the
// polyfill unless it's the broken Firefox IteratorPrototype. See
// https://github.com/facebook/regenerator/issues/274 for more details.
var _n = NativeIteratorPrototype[iteratorSymbol].call(NativeIteratorPrototype);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't this just be NativeIteratorPrototype[iteratorSymbol]()?

// polyfill unless it's the broken Firefox IteratorPrototype. See
// https://github.com/facebook/regenerator/issues/274 for more details.
var _n = NativeIteratorPrototype[iteratorSymbol].call(NativeIteratorPrototype);
if (!_n || !_n.next || _n.next.name !== 'LegacyIteratorNext') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for obscure variable names (_n). Just call it something like nativeIterator, please.

@benjamn
Copy link
Collaborator

benjamn commented Feb 15, 2017

Thanks for working on this! Please consider my requested changes, and also please evaluate @AnilRedshift's suggestion. I think I might prefer feature detection to function name testing, though I suppose this fix only needs to work in one browser.

@rayd
Copy link
Contributor Author

rayd commented Feb 16, 2017

Good points - thanks for the feedback! Hopefully getting to this tonight!

@rayd
Copy link
Contributor Author

rayd commented Feb 22, 2017

After a lot of digging, I think I understand what's happening now. Due to the changes introduced in #264, the prototype of %GeneratorPrototype% now matches the prototypes of the other iterator protoypes (i.e. the prototype of Array Iterator, Map Iterator, etc). In the problematic versions of FF, however, this prototype is the legacy Iterator protocol, which does not adhere to the standard iterator protocol. So this is why we're getting into the bad code path in FF < 45. It looks like after that version the prototype chain for iterators was updated to not include the legacy Iterator prototype.

What's really causing the problems is that calling the @@iterator function on a generator returns the legacy Iterator rather than the generator object itself. This causes us to call the incorrect next function (the legacy iterator's, instead of the generator's). In newer versions of FF and in Chrome, that call to the @@iterator function instead returns the generator object itself, which means the iterator we're using adheres to the spec in those cases. And according to the spec, calling the @@iterator function should just return this (see here), so it seems that calling the @@iterator function on a Generator should simply return the Generator object itself. There are a few solutions:

  1. Override the @@iterator function on the Generator prototype, so that it always returns the Generator object itself.
  2. Detect when we're in the above scenario and return the "right thing" from the values() function.
  3. Detect when we're in the above scenario and use the polyfill version of the iterator prototype, which shortcuts this issue.

Option 1 seems the most spec-compliant on all fronts (and is the simplest actually) and should resolve the issues, since the iterator object for "delegates" will be the Generator and not the legacy Iterator. And in my reading of the spec, I don't think defining @@iterator on the Generator prototype violates it. So I've adjusted this PR to approach the problem from this angle instead.

@intentionally-left-nil
Copy link

#1 seems like the right approach to me. Thanks for digging to the root cause

@rayd
Copy link
Contributor Author

rayd commented Mar 14, 2017

Any thoughts on this @benjamn?

@benjamn
Copy link
Collaborator

benjamn commented Mar 14, 2017

I also think the first option seems best. But, just to be clear, by "the Generator prototype" do you mean the individual prototype that each generator function gets (here), or the prototype shared by all generator objects (Gp in the runtime code)?

@intentionally-left-nil
Copy link

intentionally-left-nil commented Apr 28, 2017

Can we get this PR merged? :-) @benjamn

@benjamn benjamn merged commit ea4de31 into facebook:master Apr 28, 2017
@benjamn
Copy link
Collaborator

benjamn commented Apr 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants