ES6 usage

[lucasfcosta]

As I talked to @Joimer at #2251 I'd like to help you guys to use the new ES6 syntax, I'm creating this issue to keep track of this progress. If any of you have any considerations please don't be afraid to tell me.

If you guys want to keep compatibility with Node < 4.0 we can also set up Babel transpilation.

[Slayer95]

IMO we should be directly supporting io.js@2.

I don't plan to debug transpiled code :|

[kotarou3]

Debug natively on nodejs v5 then :|

[Slayer95]

PS main is stuck in v2 though

[Joimer]

When we can move, it is recomended to run the latest stable. When we can. Also I test on 4 and 5, no transpiling.

[lucasfcosta]

@Slayer95
Babel generates human readable code, you can also debug in more recent versions

@Joimer , are we going to keep testing on v0.12 or are we going to drop it and run Travis' build only using v4.0 and above? Since you committed ES6 code I think you may want to rebase that ( #2251 ) into #2253

[Zarel]

We're currently targetting iojs 2.x, so yes to let, but no to things like arrow functions. I agree with Slayer95 that debugging transpiled code sucks.

Versions of node/iojs newer than 2.x crash PS Main: nodejs/node#3072

[Zarel]

I think 2.x supports class in strict mode, though, so we could do that transition nowish.

[lucasfcosta]

@Zarel Ok, thanks for the clarification!

[Zarel]

@lucasfcosta, you said in the other pullreq:

I may start doing some changes this weekend then.

This worries me. To go over specific ES6 features:

• Template strings (especially tagged template strings) are currently not performant enough to use.
• Promises are currently not performant enough to use in inner loops, but is okay in less performance-intensive code. I'd recommend not converting existing code, although feel free to run specific exceptions past me.
• for...of on arrays is not performant enough to use in inner loops. I'd recommend not using it at all until performance improves, for consistency.
• for...of on Maps are probably fine. Better than manually wrangling with generators, probably. I'd suggest researching its performance to be sure.
• class is probably okay, but make sure they work in iojs 2.x
• Arrow functions are not okay, iojs 2.x doesn't support them

[lucasfcosta]

@Zarel I'll make sure to follow your considerations! You're a very instructive guy indeed, thanks.

I was thinking about starting with command-parser.js, I'd use some new String manipulation related features to simplify the code.

I'll research more about these new features' performance before implementing them.

[Slayer95]

Also note that the data folder should keep ES3 support, as long as https://github.com/Zarel/Pokemon-Showdown-Client doesn't have a full-fledged build system.

Tagging @Joimer because it's also relevant for #2251

[Zarel]

Yes, switching to String#includes instead of String#indexOf ... >= 0 is fine.

[Morfent]

Are the new object method syntax and Array.from usable? Both are available in Node 4.0.0
Edit: Array.from is undefined in iojs 2.x, but the new object methods are fine

[Slayer95]

No Object.assign, sadly.

[Zarel]

So, after refactoring some things to use class, I've discovered:

extends requires calling super() in the child constructor. Not optional, required.

super() is on average 42x slower than the equivalent ES5 trick of .call(this) on the parent constructor, and of course it's not possible to manually call the parent constructor of an ES6 class, because no clue. I mean, I can understand disallowing Class() but did you have to disallow Class.call() and Class.apply() too? Ugh...

So basically, we can't use ES6 classes on anything that requires inheritance. :|

[Slayer95]

http://kpdecker.github.io/six-speed/

[Asheviere]

So basically this only matters for the client?

[Zarel]

12x slower on Node 5.0 is a fair bit better than before... but as mentioned, we're still on iojs 2.x which is 42x slower.

[kotarou3]

How often are these objects even created? Will the speed really matter?

[Zarel]

On average, 60 times per second. At peak, probably closer to 120 times per second.

It's no inner loop, but it's not worth it for Battle and BattlePlayer objects.

[Asheviere]

It's fine for RoomGames (which is where I plan to use it), but we shouldn't
rely on inheritance for any of the core PS features.

[kotarou3]

At only 120 times per second, making it 42x slower (for something originally already very very fast) sounds insignificant to me.

This feels like premature optimisation. Have you even profiled showdown recently?

[Joimer]

I prefer dependency injection rather than inheritance. Always contain functionalities in classes that can be instanced and passed through the constructor to the class that then is going to use it. That way you also follow the single responsibility principle.

[Slayer95]

We can probably close this now?

[Zarel]

We possibly want somewhere to track which ES6 features we can and can't use...

[Zarel]

Superseded by #2419