Page MenuHomePhabricator

Put safety checks in place to prevent ES6 syntax from accidentally ending up in production.
Closed, ResolvedPublic

Description

We should probably have linting checks, maybe even at the release engineering level, to prevent ES6 code from accidentally ending up in production and/or being able to detect when this has happened.

We used to have a lot of problems with ES5 and having to support ES3 browsers and we can be pretty sure that this is going to be happen again.

Event Timeline

TheDJ created this task.Aug 30 2017, 3:17 PM

This has been done already.

I'm reopening as the current solution doesn't work to me: https://youtu.be/sqfutcKUjaQ

pmiazga reopened this task as Open.Aug 30 2017, 4:04 PM

Starting with the obvious - did you do this with a fresh npm install ?

Yes, I'm on a fresh install. @Niedzielski has same problems.

Apparently that patch was a NOOP. sourceType module will not honor the parser ecmaVersion and const and let will parse just fine... :( Which is normal, since let and const are valid JS tokens.

I'm not sure how to proceed. @Niedzielski is an eslint pro, any suggestions?

Ok weird. I did a fresh node install and now I don't see it either. Wtf...

ESLint v2 / the default ESLint parser removed blockBindings support which I believe is what we wanted.

The first suggested workaround is to set "sourceType": "script", "ecmaVersion": "5", and "allowImportExportEverywhere": true parser options but that appears to causes all imports and exports to fail. (There's also some strong wording in linked bugs against our use case.)

The second suggested workaround is to write gnarly AST rules like:

"no-restricted-syntax": [ "error", "VariableDeclaration[kind=let]", "AssignmentExpression[kind=let]" ]

Unfortunately, that only covers let. I still see other ES6 specific functionality like arrows permitted so I assume most or all of ES6 would not be flagged.

Since the parser needs to understand ES6 to use imports and exports, I don't think a singular exception can be made for import and export. Since enabling ES6 appears to turn on all other features, I don't think that using no-restricted-syntax will be durable.

Can we use require instead of import? Otherwise I recommend transpiling or great vigilance.

We can migrate to common.js back but after we made the explicit move to ES modules that would suck.

Another option we can do is run eslint also on the compiled bundle to ensure it doesn't have any es6 features (syntax or builtin usage).

In parallel, we could add babel to transpile syntax, but even so, developers need to be highly aware of the target version of JS to not use built in functions that are not available on ES5, so the same level of awareness is needed than not transpiling I would say.

The easiest/quickest option right now could be linting resources/dist/index.js with the es5 parser and env as part of npm test.

Thoughts?

TheDJ added a comment.Aug 31 2017, 9:18 AM

The easiest/quickest option right now could be linting resources/dist/index.js with the es5 parser and env as part of npm test.

That seems like a sustainable option to me. Too bad we waste some resources on a double pass, but not terrible either and at least predictable.

That seems like a sustainable option to me

Agreed. Let's do it.

Jdlrobson removed Jdlrobson as the assignee of this task.Aug 31 2017, 1:56 PM

Change 376100 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Popups@master] Run eslint on compiled assets to ensure code is es5 compatible

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

I have no idea. T174424 was so I guess...?

Change 376100 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Run eslint on compiled assets to ensure code is es5 compatible

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

Jdlrobson removed a project: Patch-For-Review.

I have uploaded OMG I am using const! to show this works. @pmiazga would you mind signing off?

Yup, will do it today.

Tested both on CI and local vagrant env. linter fails when we use ES6 keywords.

pmiazga closed this task as Resolved.Sep 8 2017, 4:34 PM