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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Unfocused, Assigned: arai)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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

5 years ago
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.).
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...
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

5 years ago
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.
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.
Depends on: 977466
Blocks: 867617
Depends on: 1083476
Summary: Convert all generators from JS1.7 generators to ES6 generators → Replace JS1.7 legacy generators with ES6 generators in Add-on SDK and chrome JS
Assignee

Comment 6

3 years ago
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.
Assignee

Updated

3 years ago
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Assignee

Updated

3 years ago
Depends on: 1321214
Assignee

Updated

3 years ago
Depends on: 1321215
Assignee

Updated

3 years ago
Depends on: 1321216
Assignee

Updated

3 years ago
Depends on: 1321217
Assignee

Updated

3 years ago
Depends on: 1321218
Assignee

Updated

3 years ago
Depends on: 1321220
Assignee

Updated

3 years ago
Depends on: 1321222
Assignee

Updated

3 years ago
Depends on: 1321225
Assignee

Updated

3 years ago
Depends on: 1321226
Assignee

Updated

3 years ago
Depends on: 1321227
Assignee

Updated

3 years ago
Depends on: 1321228
Assignee

Updated

3 years ago
Depends on: 1321229
Assignee

Updated

3 years ago
Depends on: 1321230
Assignee

Updated

2 years ago
No longer depends on: 1083476
Assignee

Comment 7

2 years ago
I'll close this bug after checking if there's remaining consumers.
Assignee

Updated

2 years ago
Depends on: 1226398
Assignee

Updated

2 years ago
Depends on: 1338248
Assignee

Updated

2 years ago
Depends on: 1338250
Assignee

Updated

2 years ago
Depends on: 1338251
Assignee

Updated

2 years ago
Depends on: 1338253
Assignee

Updated

2 years ago
Depends on: 1338254
Assignee

Updated

2 years ago
Depends on: 1338255
Assignee

Updated

2 years ago
Depends on: 1338257
Assignee

Updated

2 years ago
Depends on: 1338258
Assignee

Updated

2 years ago
Depends on: 1338262
Assignee

Updated

2 years ago
Depends on: 1338263
Blocks: 1342144
Assignee

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.