Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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

Created:
March 5, 2014, 4:38 p.m. by Thomas Greiner
Modified:
March 19, 2014, 1:36 p.m.
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
March 5, 2014, 5:11 p.m. (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 ...
March 5, 2014, 9:15 p.m. (2014-03-05 21:15:10 UTC) #2
Thomas Greiner
March 6, 2014, 11:09 a.m. (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.

Powered by Google App Engine
This is Rietveld