Closed
Bug 1119777
Opened 9 years ago
Closed 6 years ago
Remove non-standard Function.prototype.isGenerator
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: evilpie, Assigned: jandem)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, site-compat)
Attachments
(1 file)
4.77 KB,
patch
|
arai
:
review+
kmag
:
review+
|
Details | Diff | Splinter Review |
Not sure if we should really do this, because as far as I can tell there is no other way to find out if a function is a generator in ES6.
Updated•9 years ago
|
Keywords: dev-doc-needed
Comment 1•9 years ago
|
||
I guess we can check whether the given function is ES6 generator or not with following code. > let GeneratorFunction = Object.getPrototypeOf(function*(){}).constructor; > let isGenerator = f => f instanceof GeneratorFunction; > isGenerator(function(){}); false > isGenerator(function*(){}); true But it's different than Function.prototype.isGenerator with legacy generator. > isGenerator(function(){yield 1}); false So, if there is any other way to detect legacy generator (or after the removal of legacy generator support), we can remove Function.prototype.isGenerator :)
Comment 2•9 years ago
|
||
Currently Function.prototype.isGenerator is used in addon-sdk and jstests (and might be used in some add-ons). https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/test.js#52 > if (test.isGenerator && test.isGenerator()) { https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/util/sequence.js#29 > if (iterator.isGenerator && iterator.isGenerator()) https://dxr.mozilla.org/mozilla-central/source/js/src/tests/js1_8/genexps/regress-665286.js#28 > reportCompare(reported.isGenerator(), true, "reported case: is generator"); > reportCompare(simplified1.isGenerator(), true, "simplified case 1: is generator"); > reportCompare(simplified2.isGenerator(), true, "simplified case 2: is generator"); https://dxr.mozilla.org/mozilla-central/source/js/src/tests/js1_8/genexps/regress-666852.js#17 > reportCompare(reported.isGenerator(), true, "reported case: is generator"); https://dxr.mozilla.org/mozilla-central/source/js/src/tests/js1_8/genexps/regress-667131.js#14 > reportCompare(f.isGenerator(), true, desc + ": is generator"); https://dxr.mozilla.org/mozilla-central/source/js/src/tests/js1_8_5/extensions/is-generator.js#10 > reportCompare(true, "isGenerator" in Function.prototype, "Function.prototype.isGenerator present"); > ... A jit-test also uses it, but it's not required to be `isGenerator`, so we can just replace it with some other property. https://dxr.mozilla.org/mozilla-central/source/js/src/jit-test/tests/jaeger/bug704138.js#6 > reportCompare(true, "isGenerator" in Function, "Function.prototype.isGenerator present"); Then, to remove Function.prototype.isGenerator before removing legacy generator, I have following 4 plans. (Of course, if we can just remove Function.prototype.isGenerator, it's the simplest solution...) Plan A: Move `isGenerator` method to legacy generator instance, We can detect legacy generator by calling it (or just by the existence): if ("isGenerator" in f && f.isGenerator()) ... https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc46976efe62 Plan B: Define `isLegacyGenerator` property in legacy generator instance. We can detect legacy generator by the value (or just by the existence): if ("isLegacyGenerator" in f && f.isLegacyGenerator) ... This might be better than Plan A, because this prevents following bad workaround. let isGenerator = (function(){yield}).isGenerator; if (isGenerator.call(f)) ... Plan C: Add `LegacyGeneratorFunction` constructor, like `GeneratorFunction` for ES6 generator, We can detect legacy generator by following code, like comment #1: let LegacyGeneratorFunction = Object.getPrototypeOf(function(){yield}).constructor; if (f instanceof LegacyGeneratorFunction) ... https://treeherder.mozilla.org/#/jobs?repo=try&revision=998ed72422d7 Plan D: Add `IsLegacyGenerator` testing function to js shell, and use it in jstests. So, in jstests, we can detect legacy generator by it: if (IsLegacyGenerator(f)) ... But this plan has a problem with addon-sdk, if addon-sdk should handle legacy generator, this cannot be applied. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1a0149f4c9a In any way, we can remove non-standard method from standard class.
Comment 3•9 years ago
|
||
I wonder if it's worth bringing this up on es-discuss. It might not be, though. The only non-test usage (from the addon-sdk's Sequence utility) looks to me like it shouldn't use `isGenerator`: the fact that the iterator is implemented as a generator should be treated as an implementation detail. IIUC, the temptation to do these narrow checks is precisely why people are opposed to having `isFoo`-type methods.
Comment 4•9 years ago
|
||
Not sure the benefit of making it the standard :/ Anyway, for addon-sdk's case, we can test the returned value. Plan D': Check the returned value from the function call, and test the property (.next method), instead of using Function.prototype.isGenerator. https://tbpl.mozilla.org/?tree=Try&rev=88784e230164 https://treeherder.mozilla.org/#/jobs?repo=try&revision=88784e230164 (not working now?)
Comment 5•9 years ago
|
||
bug 993389 complains a problem around isGenerator. js> (function*(){}).bind(this).isGenerator() false So checking returned value would also be better for this aspect.
Comment 6•8 years ago
|
||
> let GeneratorFunction = Object.getPrototypeOf(function*(){}).constructor;
> let isGenerator = f => f instanceof GeneratorFunction;
Sadly, it doesn't work if f comes from different global :/
Updated•6 years ago
|
Keywords: site-compat
Comment 7•6 years ago
|
||
now that bug 1140759 is marked as WONTFIX, and we can just remove it at the same time as legacy generator (bug 1083482)
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Updated•6 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 9•6 years ago
|
||
Reopening as suggested in bug 1083482 comment 22.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jdemooij
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 10•6 years ago
|
||
Linux64 looks green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=052744d9467691b72cdee58325a8ee28efbb02c8&group_state=expanded
Attachment #8924697 -
Flags: review?(arai.unmht)
Comment 11•6 years ago
|
||
Comment on attachment 8924697 [details] [diff] [review] Patch Review of attachment 8924697 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! it would be better asking extra review for DeferredTask.jsm
Attachment #8924697 -
Flags: review?(arai.unmht) → review+
Updated•6 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 12•6 years ago
|
||
Comment on attachment 8924697 [details] [diff] [review] Patch Kris can review the DeferredTask.jsm change? That line was added in https://hg.mozilla.org/mozilla-central/rev/8154fcbcb707191b3526ff10d78a1ea36550213b
Attachment #8924697 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 13•6 years ago
|
||
Er, *can you review...
Comment 14•6 years ago
|
||
Comment on attachment 8924697 [details] [diff] [review] Patch Review of attachment 8924697 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/DeferredTask.jsm @@ +118,5 @@ > this._taskFn = aTaskFn; > this._delayMs = aDelayMs; > this._timeoutMs = aIdleTimeoutMs; > > + if (Object.prototype.toString.call(aTaskFn) === "[object GeneratorFunction]") { I'd rather we use `ChromeUtils.getClassName(aTaskFn) === "GeneratorFunction"`. Object.prototype.toString is pretty expensive. But it looks like that just returns "Function", so let's just remove this check entirely. It shouldn't be necessary anymore.
Attachment #8924697 -
Flags: review?(kmaglione+bmo) → review+
Comment 15•6 years ago
|
||
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a19aef803cdb Remove non-standard Function.prototype.isGenerator. r=arai,kmag
![]() |
||
Comment 16•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a19aef803cdb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 17•6 years ago
|
||
Developer release notes https://developer.mozilla.org/en-US/Firefox/Releases/58#JavaScript Reference page https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/isGenerator Compat data https://github.com/mdn/browser-compat-data/pull/602
Keywords: dev-doc-needed → dev-doc-complete
Comment 18•6 years ago
|
||
Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/legacy-generator-support-has-been-removed/
You need to log in
before you can comment on or make changes to this bug.
Description
•