Page MenuHomePhabricator

Previews cause fatal errors due to use of ES6 const keyword in non-ES6 browsers
Closed, ResolvedPublic1 Story Points

Description

MediaWiki currently still supports browsers with ES5, but it seems the ES6 keyword const has slipped into the code of page previews.

"./src/constants.js": function(e, t, n) {
            "use strict";
            var i = jQuery,
                r = i.bracketedDevicePixelRatio && i.bracketedDevicePixelRatio() || 1;
            const o = {
                off: "off",
                on: "on",
                control: "control"
            };
            t.a = o, t.b = {
                THUMBNAIL_SIZE: 300 * r,
                EXTRACT_LENGTH: 525
            }
        },

This causes fatal errors on older versions of Safari and IE.

Details

Related Gerrit Patches:
mediawiki/extensions/Popups : wmf/1.30.0-wmf.15Remove ES6 usages in Popups distribution js
mediawiki/extensions/Popups : masterDo not use keyword `const` as it's part of ES6 syntax

Event Timeline

TheDJ created this task.Aug 29 2017, 9:50 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 29 2017, 9:50 AM
TheDJ added a comment.Aug 29 2017, 9:53 AM

BTW. Linting should have caught this, and apparently didn't, so some additional configuration might be required.

TheDJ triaged this task as Normal priority.Aug 29 2017, 9:53 AM
TheDJ moved this task from Backlog to Needs Analysis on the Page-Previews board.
TheDJ renamed this task from Previews use ES6 const keyword to Previews cause fatal errors due to use of ES6 const keyword in non-ES6 browsers.Aug 29 2017, 1:20 PM

This is my fault. I made an invalid assumption.

We're using webpack where we are using EcmaScript modules. If we're using that something modern it follows we should be allowing ES6 inside our codebase. I'm surprised this compiled like this and we probably need to add a webpack plugin.

It didn't get picked up in linting because we don't lint compiled assets.

Bringing it into current sprint.

I tried to enforce ES5 but somehow ESLint doesn't force ES5. First lets talk about it, then if necessary I'll create a follow-up phab ticket to enforce ES5 in ESLint or any other possible way

Change 374552 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/Popups@master] Do not use keyword const as it's part of ES6 syntax

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

@TheDJ you're right, but probably it requires some conversation before we enable es6->es5 transpilation.

Change 374552 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Do not use keyword const as it's part of ES6 syntax

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

phuedx removed pmiazga as the assignee of this task.Aug 29 2017, 3:45 PM
phuedx moved this task from To Do to Ready for Signoff on the Readers-Web-Kanbanana-Board-Old board.
phuedx set the point value for this task to 1.
phuedx added a project: Unplanned-Sprint-Work.
phuedx added a subscriber: pmiazga.
phuedx added a subscriber: phuedx.

^ Being bold.

This probably should be swat deployed.

If swatting this, also include https://gerrit.wikimedia.org/r/374376, which removes another use of an ES6 feature.

In case wondering, eslint does not complain as parserOptions is set to sourceType "module" (for ES6 modules).

@Jdlrobson it does not complain when you set parserOptions.ecmaVersion: 5 and env.es6: false.

Change 374582 had a related patch set uploaded (by Jdlrobson; owner: Bartosz Dziewoński):
[mediawiki/extensions/Popups@wmf/1.30.0-wmf.15] Remove ES6 usages in Popups distribution js

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

Baha has kindly volunteered to SWAT in the window later today.

Change 374582 merged by jenkins-bot:
[mediawiki/extensions/Popups@wmf/1.30.0-wmf.15] Remove ES6 usages in Popups distribution js

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

bmansurov removed bmansurov as the assignee of this task.Aug 29 2017, 11:37 PM
bmansurov added a subscriber: bmansurov.

The change has been SWAT deployed. I don't have the problematic browsers handy, but I can confirm that the const keyword is gone from https://en.wikipedia.org/w/load.php?modules=ext.popups and popups are working on enwiki for example.

Jdlrobson closed this task as Resolved.Aug 30 2017, 1:32 PM
Jdlrobson claimed this task.

I can verify this later today if nobody beats me to it. We can check if Popups work for IE9 in browser stack to resolve this.

Jdlrobson reopened this task as Open.Aug 30 2017, 1:32 PM

sorry was a bit premature there :)

I can also confirm it works on Opera 12 again. I think you can safely resolve this.

Jdlrobson closed this task as Resolved.Aug 30 2017, 2:15 PM

Thank you!