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.
| Subject | Author | Repo | Branch | Lines +/- | |
|---|---|---|---|---|---|
| Run eslint on compiled assets to ensure code is es5 compatible | Jdlrobson | mediawiki/extensions/Popups | master | +68 -10 |
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | Jdlrobson | T174424 Previews cause fatal errors due to use of ES6 const keyword in non-ES6 browsers | |||
| Resolved | pmiazga | T174570 Put safety checks in place to prevent ES6 syntax from accidentally ending up in production. |
This has been done already.
https://gerrit.wikimedia.org/r/#/q/Id1d147866c37de3794213378a5e8a08af6f796a1
I'm reopening as the current solution doesn't work to me: https://youtu.be/sqfutcKUjaQ
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?
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?
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.
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
Change 376100 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Run eslint on compiled assets to ensure code is es5 compatible
I have uploaded OMG I am using const! to show this works. @pmiazga would you mind signing off?