search

phetsims / chipper

Tools for developing and building PhET interactive simulations.
http://scenerystack.org/
MIT License
11 stars 14 forks source link

Should we use ES6 in simulation code? #551

Closed samreid closed 6 years ago

samreid commented 7 years ago

@jonathanolson pointed out in https://github.com/phetsims/axon/issues/118#issuecomment-283426535 that the caniuse table for ES6 Proxy looks like it covers the bases for our supported platforms:

image

While I was there, I noticed that ES6 Arrow Functions, const/let, and classes are also green for our platforms. Should we go for it? If so, how much a risk do we run of clients running browsers that do not support it? Would we put language level checks before starting up the simulation? Should we check our analytics to get a better understanding of how many clients we would lose if we started requiring modern ES6 constructs in our production code?

Of course it may be better for us to stick with ES5 code "indefinitely" to avoid questions like the above, but on the other hand, when the same question came up regarding moving from ES4, I believe we made the right decision to jump in to ES5, and it is has helped us immensely (get methods, use strict, array methods, json, etc) and has been worth the cost.

@jonathanolson can you comment on this before we bring it to the wider development team?

jonathanolson commented 6 years ago

@jonathanolson can you think of why IE11 doesn't like Promise? Is this expected?

Hmm, if we want to use promises, we may need to include a polyfill. Tagging for developer meeting to see if that is something people would want.

zepumph commented 6 years ago

In the mean time, I stripped out the promises in https://phet-dev.colorado.edu/html/chains/1.18.0-es6.3/phet/chains_all_phet.html. In this version there are arrow functions, let/const, and template vars. I think that would get us pretty far just with that. We could pass that off to QA now, because after some searching it seems like a library is required to polyfill Promises for ie11, and I'm unsure why we would need that feature in our phetsims.

EDIT: spelling

samreid commented 6 years ago

Hmm, if we want to use promises, we may need to include a polyfill. Tagging for developer meeting to see if that is something people would want.

In my opinion, continuation passing style is very clear and works great for simulations. Not sure if we need promises or async/await there (that's more important in the build server or perhaps test harnesses).

jbphet commented 6 years ago

I haven't found the need for using promises in sims since we generally don't do async requests to outside resources. Promises are used extensively in the translation utility, so I'm pretty familiar with them. I think we could decide not to use them for now and if there ever came a strong need we could use a polyfill for IE11. I looked around on line, and it sounds like there are several good ones available.

jessegreenberg commented 6 years ago

6/28/18 meeting:

@ariel-phet said that if it is not supported in IE11 we should really try not to use Promise. In general, we agree that Promise is not desirable in sim code. We should add assertions that make sure we are not using Promises.

So next step is for @zepumph to create a QA task to test the changes described in https://github.com/phetsims/chipper/issues/551#issuecomment-400412369. If everything checks out, we will merge the branches that @zepumph is working in with master (es6-chains and es6-chipper).

As part of exploration, we should have a file enumerating es6 features and example usages. Developers can learn from this and we can go over this list in a future dev meeting. This file will exist in a new repo that is runnable/buildable and will have experimental code and features to play with. @andrealin will come up with a name for this repo, and @jonathanolson will work on the es6 related content.

jonathanolson commented 6 years ago

Should included Classes, and specifically cases where we extend our usual types with classes (e.g. inheriting Rectangle).

zepumph commented 6 years ago

So next step is for @zepumph to create a QA task to test the changes described in #551 (comment). If everything checks out, we will merge the branches that @zepumph is working in with master (es6-chains and es6-chipper).

Next steps seem to be that @jonathanolson is going to work on getting together a group of es6 examples in a new repo, building it, and then bringing it back to devs to comment on that set of features we want to support. From there we will hand it over to QA. Interim testing can be done by @jonathanolson by running the built code in IE to see if any problems arise like Promise did for me.

jonathanolson commented 6 years ago

I have a branch (https://github.com/phetsims/wilder/issues/1) set up that I'll use for enumerating/testing planned supported features.

jonathanolson commented 6 years ago

@lmulhall-phet, I deployed a version with what I think the recommended ES6 support should include (along with things that error out if it isn't supported properly) in https://phet-dev.colorado.edu/html/wilder/1.0.0-es6.1/phet/wilder_en_phet.html. Should I open a QA issue for that?

samreid commented 6 years ago

Looks great @jonathanolson. If I understand it correctly, the errors will report to the console and testing will require using dev tools/console and/or tethering (for iPad). Not sure about chromebooks. @jonathanolson is that what you intended? Should we add an alert on error to make it easier to spot errors? Or perhaps a console.log('there were no errors') at the end of running the code to make sure the QA team sees something in the console? What do you recommend?

jonathanolson commented 6 years ago

I'll add a visual "text" when there are no errors, and will get a new deployment.

jonathanolson commented 6 years ago

Actually, nevermind. If it is broken, it will stay on the "loading" screen. If the sim loads, it worked.

ghost commented 6 years ago

Should I open a QA issue for that?

@jonathanolson, please. Or I can, but I'm not sure what exactly you want tested.

jonathanolson commented 6 years ago

@jonathanolson, please. Or I can, but I'm not sure what exactly you want tested.

Done, see https://github.com/phetsims/QA/issues/139.

jonathanolson commented 6 years ago

I'll look into babel-polyfill to see if we can get Map/generators, and how much space it would take (if we can get less than 100kB) and will discuss with @ariel-phet next week.

pixelzoom commented 6 years ago

It was also proposed at 7/5/18 dev meeting that we start by introducing ES6 into sim-specific code, to evaluate and identify issues. And in the meantime keep ES6 out of common code.

samreid commented 6 years ago

https://github.com/phetsims/QA/issues/139 indicates that https://phet-dev.colorado.edu/html/wilder/1.0.0-es6.1/phet/wilder_en_phet.html passed on all tested platforms.

jonathanolson commented 6 years ago

I've looked into the polyfills, and it looks like it may be beneficial to use a custom build of core-js to polyfill only the features we want polyfilled (less file size, and lower chance of things breaking). Adding in the other part of babel-polyfill (basically the regenerator-runtime, for generators) could be done if we are interested in including generators, but it could potentially be omitted (I'd potentially want to check on performance implications of using the generator polyfill).

jonathanolson commented 6 years ago

Trying to do a custom build is failing so far, and doesn't seem to be documented well.

So, would Map/Set/Promise/etc. be worth ~100kB?

samreid commented 6 years ago

After equality explorer gets its SHAs, this chipper change will be merged to master, then we are approved to use this in simulations, including Wave Interference.

pixelzoom commented 6 years ago

7/12/18 dev meeting:

samreid commented 6 years ago

Sam Reid [3:21 PM] It seems @pixelzoom has SHAs for EqEx, can I merge chipper es6 branch to master for https://github.com/phetsims/chipper/issues/551#issuecomment-404606004 ?

Jonathan Olson [3:22 PM] Sure. If you merge it, can you record how you modify Intellij so that the language level works fine for inspections/etc.?

Chris Malley [3:23 PM] RCs are published for all EqEx sims, so you may proceed with merging things into master.

samreid commented 6 years ago

There are no merge conflicts, and aqua tests show now systemic problems. I'll push and monitor Bayes CT for issues.

samreid commented 6 years ago

Chris Malley [4:10 PM] Now that es6 changes are in master… Is es6-paths out of sync?

Sam Reid [4:10 PM] Perhaps people should add paths to that file when they opt in to ES6?

Jonathan Olson [4:10 PM] es6-paths is presumably not needed anymore?

Sam Reid [4:11 PM] Ah I see, checking…

Chris Malley [4:11 PM] but don’t we still want to keep es6 out of common code? at least temporarily?

Jonathan Olson [4:11 PM] ahh, you want that to be guaranteed by our ide/linter?

Chris Malley [4:12 PM] it’s either that or the honor system :)

Jonathan Olson [4:12 PM] it seemed that the honor system was the plan to me

Chris Malley [4:12 PM] “It wasn’t me, your Honor.“ :)

Jonathan Olson [4:12 PM] I wouldn’t want to spent a bunch of time having everyone do IDE modifications so only common code can’t have it and do the linting/other changes it seems CCK-black-box-study doesn’t have screenshots in its release branch. should we have screenshots for it?

Sam Reid [4:19 PM] It is not intended for website publication I’ll proceed with the honor system strategy. If that is failing us, we can re-evaluate.

samreid commented 6 years ago

Chris Malley [4:25 PM] OK. So delete es6-paths, and set WebStorm to “ECMAScript 6”?

Sam Reid [4:25 PM] Yes, I’ll push those changes shortly