Linter set to ES5 - Githubissues

NovemLinguae commented 2 years ago

I notice our linter is set to enforce ES5 and reject any JavaScript features newer than that. This is a little inconvenient to the developer, who has to avoid newer language features such as match, let, class, await, startsWith, endsWith, includes, regex s flag, etc.

Was just wondering what the idea behind keeping the linter on ES5 is. I know Twinkle has 45,000 users or so. I guess the assumption is that a small subset of these folks are using very old browsers that don't support ES6+?

Is it worth the tradeoff? Should we plan to allow ES6 sometime in the future?

siddharthvp commented 2 years ago

The reason is just that MW gadgets don't yet support ES6. We need https://phabricator.wikimedia.org/T75714 to be resolved first one way or another. It is quite hard to resolve satisfactorily - as it requires introducing a ES6-capable validator. I have a rather hacky patch filed as an interim solution.

NovemLinguae commented 1 year ago

This patch was +2'd today, opening the door for gadgets to start using ES6. Next Thursday when this deploys, do we want to switch Twinkle over to ES6?

If so, here is our todo list:

[ ] Make a post at the interface administrator's noticeboard requesting this change
[ ] Reconfigure our linter to allow ES6

siddharthvp commented 1 year ago

I am on board with this. If @MusikAnimal is also in agreement, @NovemLinguae can you please file the patch for updating the linter reconfiguration, gadget definition file and the README? On-wiki gadget definition change can be handled as part of deployment and IMO doesn't need an IANB post.

MusikAnimal commented 1 year ago

100% in support! We may kill IE support in doing this, but nowadays that's the intention: https://www.mediawiki.org/wiki/Compatibility/IE11

I'm happy to do the deployment when the time comes. Just ping me.

NovemLinguae commented 1 year ago

I see a bunch of JavaScript versions here. Are these all safe for Twinkle/gadget use now? Or do we need to look into having the linter continue enforcing a block of some of the newer ones?

siddharthvp commented 1 year ago

We're still limited by what the minifier supports, which is ES6 == ES2015 (see https://phabricator.wikimedia.org/T277675). However, we're still able to use Array.includes(), Object.values(), etc which are merely new methods rather than new syntax.

NovemLinguae commented 1 year ago

On Thursday, I think we want to change https://en.wikipedia.org/wiki/MediaWiki:Gadgets-definition

to

Right? The change is adding |requiresES6

Can make the change once group2 goes to wmf.7 - https://versions.toolforge.org/

siddharthvp commented 1 year ago

Yes

MusikAnimal commented 1 year ago

Done!

NovemLinguae commented 1 year ago

@MusikAnimal. Thank you my friend. Can you also set |requiresES6 for Morebits.js? I think we decided to do that in the pull request.

MusikAnimal commented 1 year ago

Can you also set |requiresES6 for Morebits.js? I think we decided to do that in the pull request.

Is that going to break any other scripts that use Morebits? As in, after we deploy any ES6-only code? I know this isn't MediaWiki proper, but libraries shouldn't normally use ES6 (for now). Maybe we should have a Babel build step for Morebits

siddharthvp commented 1 year ago

The issue would be that we would need a separate lint config for morebits, and I don't think eslint supports file-specific override for ecmaVersion (though I suppose we could put morebits in a subdir and put another .eslintrc in there).

Morebits isn't much used outside Twinkle. I think the biggest use is DYK-helper with 340 users (which is written by myself - and I'm fine with not having IE11 support for that), the next biggest use is arb.js with 70 users, which is dependent on Twinkle so won't work on IE11 anyway with Twinkle requiring ES6. So I think there's little point in retaining IE11 support in morebits and the costs are not justified.

MusikAnimal commented 1 year ago

There's a few more I found with Global Search (only enwiki listed below, but there are other results, assuming the ES6 migration will eventually carry over cross-wiki):

Some of these seem to be custom extensions to Twinkle/Morebits, which may mean they were fragile or prone to breakage to begin with. I didn't thoroughly review these scripts so it's also possible they won't actually break (maybe only gadgets would?). At any rate, I've no issue with making Morebits ES6-only, but I don't think we should cause breakage, even for personal JS, without some sort of heads up.

In short, if you all are confident, I am too :)

MusikAnimal commented 1 year ago

The issue would be that we would need a separate lint config for morebits, and I don't think eslint supports file-specific override for ecmaVersion (though I suppose we could put morebits in a subdir and put another .eslintrc in there).

Frankly, I've always wondered why Morebits wasn't set up that way. It seems odd to have the source files for a project of this size in the parent directory. A lib/ directory or something might be more appropriate, given the intention of Morebits. I'm not involved or particularly interested in maintaining Morebits as a standalone library, but if others wish it to be that way, it might even make sense as a separate repository. Overall I think the long-term future of Twinkle should look to OOUI and Codex, but I digress.

NovemLinguae commented 1 year ago

Per this onwiki discussion, Morebits was set to |requiresES6 today.

wikimedia-gadgets / twinkle

Linter set to ES5 #1515