Page MenuHomePhabricator

CentralNotice failing in older browsers due use of ECMAScript 6 syntax
Closed, ResolvedPublic1 Estimated Story Points

Description

We got a report of permanent JS problems on IE 11 on ro.wp [1], which in theory [2] is still supported. See comment below for details on the console output. This seems to be different from the wikidata issue reported at [3] (at least I get a different error)

[1] https://ro.wikipedia.org/wiki/Wikipedia:Cafenea#Probleme_cu_interfa.C8.9Ba
[2] https://www.mediawiki.org/wiki/Compatibility#Browser_support_matrix
[3] https://www.wikidata.org/wiki/Wikidata:Contact_the_development_team#A_bug.3F

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I also get this in debug mode. Blows up on this in /CentralNotice/resources/subscribing/ext.centralNotice.display.js:

		/**
		 * Register that the current page view is included in a test.
		 * @param {string} identifier A string to identify the test. Should not contain
		 *   commas.
		 */
		registerTest( identifier ) {
			cn.internal.state.registerTest( identifier );
		},

This looks like obviously invalid syntax to me… perhaps it does something in ES6 that IE 11 doesn't support, but it probably doesn't do what the author intended.

Seems to be caused by https://gerrit.wikimedia.org/r/#/c/290361/4

(That patch introduces the same kind of mistake in resources/subscribing/ext.centralNotice.display.state.js, too. This is apparently ES6 "concise methods" syntax, so it actually does what the author intended, but it's only supported by some browsers, causing a syntax error in others.)

Form what I see here, this also affects Safari and older FF and Chrome versions.

Strainu renamed this task from Issues with JavaScript on IE11 on Romanian Wikipedia to EcmaScript 6 features are not supported in older browsers.May 27 2016, 1:32 PM

This is also affecting UploadWizard, which I maintain, and people are starting to blame it (https://commons.wikimedia.org/wiki/Commons:Upload_help#Can.27t_use_new_upload_wizard), and no one responsible for CentralNotice seems available (I think the whole team is in the US and probably still asleep). I'm going to see if I can fix it.

Change 291246 had a related patch set uploaded (by Alex Monk):
Revert "Update CentralNotice submodule"

https://gerrit.wikimedia.org/r/291246

Change 291246 merged by jenkins-bot:
Revert "Update CentralNotice submodule"

https://gerrit.wikimedia.org/r/291246

Mentioned in SAL [2016-05-27T15:02:01Z] <krenair@tin> Synchronized php-1.28.0-wmf.3/extensions/CentralNotice/resources/subscribing: rv due to T136387 (duration: 00m 36s)

matmarex edited projects, added Fundraising-Backlog; removed Patch-For-Review.
matmarex added a subscriber: Krenair.

Deployment of the faulty patch (T134286) was reverted by @Krenair and the issue should no longer be occurring right now. The bug still needs to be fixed in CentralNotice to prevent it from re-occurring after the next deployment on Tuesday, so I am not closing this task.

I can confirm JavaScript now works in IE 11. Waiting for confirmation on the other browsers.

I can confirm JavaScript now works again in Opera 12.18 (released Feb 2016).

Deployment of the faulty patch (T134286) was reverted by @Krenair and the issue should no longer be occurring right now. The bug still needs to be fixed in CentralNotice to prevent it from re-occurring after the next deployment on Tuesday, so I am not closing this task.

It should be coincided with a change to CentralNotice's JSHint configuration to ensure ES6 syntax is considered invalid - as to avoid regressions in the future.

Krinkle renamed this task from EcmaScript 6 features are not supported in older browsers to CentralNotice failing in older browsers due use of ECMAScript 6 syntax.May 27 2016, 4:06 PM

It should be coincided with a change to CentralNotice's JSHint configuration to ensure ES6 syntax is considered invalid - as to avoid regressions in the future.

Yeah, T136408: Update CentralNotice JSHint config to restrict syntax to ES3 (disallow ES5 or ES6) was filed about that.

Change 291264 had a related patch set uploaded (by Ejegg):
jshint, gruntfile, and compatibility fixes

https://gerrit.wikimedia.org/r/291264

Change 291264 merged by jenkins-bot:
jshint, gruntfile, and compatibility fixes

https://gerrit.wikimedia.org/r/291264

matmarex assigned this task to Ejegg.
matmarex removed a project: Patch-For-Review.

I see this in production again. Is it just me? Was the faulty patch re-deployed?

Change 292279 had a related patch set uploaded (by Bartosz Dziewoński):
Revert "Update CentralNotice submodule"

https://gerrit.wikimedia.org/r/292279

CentralNotice seems to have some unusual deployment method that I don't understand. Can you folks please take care of doing whatever needs to be done to ensure this doesn't regress again?

Aaaaarg!! Fortunately, it only made it to groups 0 and 1, so not to WP.

Change 292279 merged by jenkins-bot:
Revert "Update CentralNotice submodule"

https://gerrit.wikimedia.org/r/292279

Change 292282 had a related patch set uploaded (by Ejegg):
Merge branch 'master' into wmf_deploy

https://gerrit.wikimedia.org/r/292282

Change 292285 had a related patch set uploaded (by Ejegg):
Revert "ext.centralNotice.display: API for registering tests"

https://gerrit.wikimedia.org/r/292285

Change 292285 merged by Ejegg:
Revert "ext.centralNotice.display: API for registering tests"

https://gerrit.wikimedia.org/r/292285

Sorry! https://gerrit.wikimedia.org/r/292285/ reverts it in the wmf_deploy branch, source of the -wmf.x branch submodules. Next merge into wmf_deploy will have ES3 enforcement in effect.

Change 292282 abandoned by Ejegg:
Merge branch 'master' into wmf_deploy

Reason:
too much, too soon

https://gerrit.wikimedia.org/r/292282

@mmodell The revert doesn't appear to be live in production.

Change 292304 had a related patch set uploaded (by Awight):
Update extensions/CentralNotice submodule

https://gerrit.wikimedia.org/r/292304

Change 292304 merged by jenkins-bot:
Update extensions/CentralNotice submodule

https://gerrit.wikimedia.org/r/292304

Mentioned in SAL [2016-06-02T00:26:39Z] <awight@tin> Synchronized php-1.28.0-wmf.4/extensions/CentralNotice: Fix for T136387 (duration: 00m 38s)

Change 292368 had a related patch set uploaded (by AndyRussG):
jshint, gruntfile, and compatibility fixes (partial, wmf_deploy branch only)

https://gerrit.wikimedia.org/r/292368

Change 292401 had a related patch set uploaded (by AndyRussG):
Revert "jshint, gruntfile, and compatibility fixes"

https://gerrit.wikimedia.org/r/292401

Change 292402 had a related patch set uploaded (by AndyRussG):
Re-apply "jshint, gruntfile, and compatibility fixes" (part 1)

https://gerrit.wikimedia.org/r/292402

Change 292403 had a related patch set uploaded (by AndyRussG):
Re-apply "jshint, gruntfile, and compatibility fixes" (part 2)

https://gerrit.wikimedia.org/r/292403

Change 292404 had a related patch set uploaded (by AndyRussG):
Re-apply "jshint, gruntfile, and compatibility fixes" (part 1)

https://gerrit.wikimedia.org/r/292404

50b7a37df47a1ef798efdfc1186d66c9d75c1d51 is the current rev deployed for CentralNotice. A spot check shows that it is the version that is live on the application servers.

Can this be removed as a wmf.4 blocker?

Hi @thcipriani! Yes, that commit is safe to deploy!! Thx :)

Change 292368 abandoned by AndyRussG:
jshint, gruntfile, and compatibility fixes (partial, wmf_deploy branch only)

Reason:
See instead I91de8b2dfa

https://gerrit.wikimedia.org/r/292368

Change 292401 abandoned by AndyRussG:
Revert "jshint, gruntfile, and compatibility fixes"

Reason:
Unneeded, was part of planned administrative curiosities

https://gerrit.wikimedia.org/r/292401

Change 292402 abandoned by AndyRussG:
Re-apply "jshint, gruntfile, and compatibility fixes" (part 1)

Reason:
Unneeded, was part of planned administrative curiosities

https://gerrit.wikimedia.org/r/292402

Change 292403 abandoned by AndyRussG:
Re-apply "jshint, gruntfile, and compatibility fixes" (part 2)

Reason:
Unneeded, was part of planned administrative curiosities

https://gerrit.wikimedia.org/r/292403

Change 292404 merged by jenkins-bot:
Re-apply "jshint, gruntfile, and compatibility fixes" (part 1)

https://gerrit.wikimedia.org/r/292404

Change 292503 had a related patch set uploaded (by Awight):
Update extensions/CentralNotice submodule

https://gerrit.wikimedia.org/r/292503

Mentioned in SAL [2016-06-02T23:59:36Z] <awight@tin> Started scap: Deploying labtestwiki AuthManager config; Enabling Popups experiment; CentralNotice fixes for T136408, T136387; Special:Notifications fixes

Mentioned in SAL [2016-06-03T00:24:45Z] <awight@tin> Finished scap: Deploying labtestwiki AuthManager config; Enabling Popups experiment; CentralNotice fixes for T136408, T136387; Special:Notifications fixes (duration: 25m 08s)

AndyRussG moved this task from Doing to Done on the Fundraising Sprint Killing Time board.