-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
… form and don't use it. Polyfill instead.
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); |
There was a problem hiding this comment.
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') { |
There was a problem hiding this comment.
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.
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. |
Good points - thanks for the feedback! Hopefully getting to this tonight! |
…self is returned.
After a lot of digging, I think I understand what's happening now. Due to the changes introduced in #264, the prototype of What's really causing the problems is that calling the
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 |
#1 seems like the right approach to me. Thanks for digging to the root cause |
Any thoughts on this @benjamn? |
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 ( |
Can we get this PR merged? :-) @benjamn |
Attempt to address #274. We're basically checking whether or not the
NativeIteratorPrototype
's next function is the badLegacyIteratorNext
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?