968038 - Replace JS1.7 legacy generators with ES6 generators in Add-on SDK and chrome JS

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

We use JS1.7 style generators all over the show. We really should be using ES6 generators, which have slightly different syntax and semantics. Because ES6 is the One True Path. And by that I mean ES6 generators are the universal standard that will get all the performance tweaks, meanwhile JS1.7 generators will eventually be deprecated.

*Not* calling this a good-first-bug, unless someone disagrees. Because, frankly, too few people are familiar enough with both types of generators :\

Comment 1

[:Gijs (out for now; he/him)]

I wouldn't call it a good first bug, but you could probably mentor people to do per-directory patches (ie first browser/base, then browser/components/foo, bar, toolkit/content, etc.).

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

We should totally do that. I however fear that we will almost never finish this task as there probably are too many people still using the old syntax and writing new tests and code with it (me included if I don't remind myself every time).

Can we implement something that would somehow keep us from writing new code using legacy generator syntax? A push hook feels complicated and maybe a little too harsh...

Comment 3

[Mike Conley (:mconley) (:⚙️)]

For patches reviewed using our Review Board instance[1], we could write a Review Bot extension that looks for and opens issues for old generator syntax.

Just a possibility. :)

[1]: http://reviewboard.allizom.org/

Comment 4

[:Gijs (out for now; he/him)]

So, first of all, confessing my lack of knowledge here, do we have documentation about the differences? Simple searches turn up things that look the same to me (at least at 1.30 in the morning): functions using the yield keyword to generate values. What are the particular differences that need refactoring? A document about this on MDN and a post to fx-team and m.d.platform would probably be a good start.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Old generators do return a value from gen.next() and throw StopIteration if they're out of values. New generators return {value: 123, done: false/true} from gen.next(). As long as we're using for..of to iterate over generator values we're good but if anything is using .next() then the transition might not be as easy.

It looks like there's quite a few places using .next() and fixing (or rather finding) the generator clients is a lot harder than just finding a function that contains a yield stmt.

And yes, our MDN article is unfortunately a little behind. We really need some updated docs on this.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: List of the position of yield in legacy generator

the attachment is a list of legacy generator's yield's position, collected by the following try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5a5c3ca063b1a4a0477c52309adea1a254468e8

note that the position may be eval-ed code's line/column.

Comment 7

[Tooru Fujisawa [:arai]]

I'll close this bug after checking if there's remaining consumers.