Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(938)

Issue 6322148272504832: Fixed: Transpiling of for-each in notification.js leads to error due to array modifications (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 8 months ago by Thomas Greiner
Modified:
5 years, 8 months ago
Reviewers:
Felix Dahlke
Visibility:
Public.

Description

The ES6 to ES5 translation for Chrome/Opera/Safari leads to problems when used in combination with listeners for the new question-type notifications if a listener is removed from within a listener (see http://codereview.adblockplus.org/5749582424178688/).

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -1 line) Patch
M lib/notification.js View 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 3
Thomas Greiner
5 years, 8 months ago (2014-03-05 17:11:19 UTC) #1
Felix Dahlke
LGTM for now, but IMO we should fix this in the build process and then ...
5 years, 8 months ago (2014-03-05 21:15:10 UTC) #2
Thomas Greiner
5 years, 8 months ago (2014-03-06 11:09:24 UTC) #3
On 2014/03/05 21:15:10, Felix H. Dahlke wrote:
> LGTM for now, but IMO we should fix this in the build process and then revert
> this. Feel free to create a follow-up for that on Trello.

I wouldn't consider this a problem with the process itself since this is a
logical and not a syntactical problem.

I might not have made it clear enough in the issue description but the problem
is that the "id" property of "listeners" is being removed which means that the
array is no longer available for the next iteration because "listeners[id]" is
refering to undefined at that point. That's why introducing a new variable fixes
that problem. We could automatically do that in JSHydra of course but it doesn't
seem like it belongs in there.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5