Support for static properties on es6 classes

[zepumph]

We have dropped support for IE11 (see issues like #1037). This caused the question of supporting static "getter" fields (not functions) on es6 classes. See discussion in phetsims/tandem#188 (comment)

Tagging @pixelzoom and @samreid.

@samreid mentioned that over in https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Public_class_fields there is support for our browsers, and perhaps babel has an easy plugin to support this https://babeljs.io/docs/en/babel-plugin-proposal-class-properties.

I think of this issue as adding support for two things:

Static fields

class MyParty {
  static hasFun = 'duh';
}

Static Getters

class MySuperParty {
  static get hasFun() {
    return 'absolutely';
  }
}

I think the steps for this issue could be:

1. At dev meeting, decide if we want this experimental feature enough to work on it.
2. Create a built-sim example of an es6 class with some static fields and getters for properties. (likely in Wilder)
3. Build that, and pass see if it already works for our target devices (may need QA time to confirm on all platforms if we get that far).
4. If this doesn't work, investigate the plugin mentioned above for supporting this if needed.\

Marking for dev meeting to decide if this is worth investigation.

[zepumph]

I want to note that this is not a top-priority issue, but it is annoying to have to work around this non-obvious defect with what our codebase supports.

[zepumph]

After a conversation with @ariel-phet, I updated the first comment, and marked for developer meeting.

[samreid]

I tried adding

static x = true;

to Bunny.js. I observed:

• This runs properly in unbuilt mode
• This triggers a lint error
• grunt build fails with this error:

~/apache-document-root/main/natural-selection$ grunt --brands=phet --lint=false
Running "report-media" task

Running "clean" task

Running "build" task
Building runnable repository (natural-selection, brands: phet)
Building brand: phet
Webpack build errors: [
  ModuleParseError: Module parse failed: Unexpected token (41:11)
  You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
  | class Bunny extends Organism {
  | 
  >   static x = true;
  | 
  |   /**
    at handleParseError (/Users/samreid/apache-document-root/main/chipper/node_modules/webpack/lib/NormalModule.js:469:19)
    at /Users/samreid/apache-document-root/main/chipper/node_modules/webpack/lib/NormalModule.js:503:5
    at /Users/samreid/apache-document-root/main/chipper/node_modules/webpack/lib/NormalModule.js:358:12
    at /Users/samreid/apache-document-root/main/chipper/node_modules/loader-runner/lib/LoaderRunner.js:373:3
    at iterateNormalLoaders (/Users/samreid/apache-document-root/main/chipper/node_modules/loader-runner/lib/LoaderRunner.js:214:10)
    at Array.<anonymous> (/Users/samreid/apache-document-root/main/chipper/node_modules/loader-runner/lib/LoaderRunner.js:205:4)
    at Storage.finished (/Users/samreid/apache-document-root/main/chipper/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:55:16)
    at /Users/samreid/apache-document-root/main/chipper/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:91:9
    at /Users/samreid/apache-document-root/main/chipper/node_modules/graceful-fs/graceful-fs.js:123:16
    at FSReqCallback.readFileAfterClose [as oncomplete] (internal/fs/read_file_context.js:63:3)
  
]
Fatal error: Converting circular structure to JSON
    --> starting at object with constructor 'NormalModule'
    |     property 'dependencies' -> object with constructor 'Array'
    |     index 0 -> object with constructor 'HarmonyCompatibilityDependency'
    --- property 'originModule' closes the circle

Next I tried adding:

static get x() {return true;}

• This worked in unbuilt mode
• This triggered a lint error (bad-text for static get, which we can easily repair)
• This worked in built mode

[samreid]

I got this to work, but I had to use the babel-loader in webpack in addition to updating the babel step itself. Maybe if we use the webpack babel-loader we will no longer need the babel post-processing step? Note: this fix gets builds working but not lint.

Index: main/chipper/js/grunt/transpile.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- main/chipper/js/grunt/transpile.js	(revision 06ea944f1622bd2fcb04a05cd0d3b5281b8259bf)
+++ main/chipper/js/grunt/transpile.js	(date 1601503894065)
@@ -36,6 +36,9 @@
     // the 'true' option was faster by a hair.
     compact: true,
 
+    // https://babeljs.io/docs/en/babel-plugin-proposal-class-properties
+    plugins: [   '../chipper/node_modules/@babel/plugin-proposal-class-properties' ],
+
     // Use chipper's copy of babel-preset-env, so we don't have to have 30MB extra per sim checked out.
     presets: [ [ '../chipper/node_modules/@babel/preset-env', {
 
Index: main/chipper/package.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- main/chipper/package.json	(revision 06ea944f1622bd2fcb04a05cd0d3b5281b8259bf)
+++ main/chipper/package.json	(date 1601504231377)
@@ -10,7 +10,9 @@
     "archiver": "~3.1.1",
     "@babel/core": "~7.8.6",
     "@babel/preset-env": "~7.8.6",
+    "@babel/plugin-proposal-class-properties": "~7.10.4",
     "babel-eslint": "^10.0.3",
+    "babel-loader": "^8.1.0",
     "eslint": "^6.8.0",
     "eslint-plugin-react": "^7.18.0",
     "grunt": "~1.1.0",
Index: main/natural-selection/js/common/model/Bunny.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- main/natural-selection/js/common/model/Bunny.js	(revision 3e8cf4aaf5eafc77789ed9224e671bfe9ba97957)
+++ main/natural-selection/js/common/model/Bunny.js	(date 1601504382300)
@@ -399,6 +399,8 @@
 
     this.validateInstance();
   }
+
+  static testName = 'hello';
 }
 
 /**
@@ -444,5 +446,7 @@
   applyState: ( bunny, stateObject ) => bunny.applyState( stateObject )
 } );
 
+console.log(Bunny.testName);
+
 naturalSelection.register( 'Bunny', Bunny );
 export default Bunny;
\ No newline at end of file
Index: main/chipper/js/grunt/webpackBuild.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- main/chipper/js/grunt/webpackBuild.js	(revision 06ea944f1622bd2fcb04a05cd0d3b5281b8259bf)
+++ main/chipper/js/grunt/webpackBuild.js	(date 1601504284690)
@@ -98,7 +98,27 @@
       // Exclude brand specific code. This includes all of the `phet-io` repo for non phet-io builds.
         ( brand === 'phet' ? [ ignorePhetioBrand, ignorePhetioRepo, ignoreAdaptedFromPhetBrand ] :
           brand === 'phet-io' ? [ ignorePhetBrand, ignoreAdaptedFromPhetBrand ] :
-          brand === 'adapted-from-phet' ? [ ignorePhetBrand, ignorePhetioBrand, ignorePhetioRepo ] : [] )
+          brand === 'adapted-from-phet' ? [ ignorePhetBrand, ignorePhetioBrand, ignorePhetioRepo ] : [] ),
+
+      module: {
+        rules: [
+          {
+            test: /\.(mjs|js|jsx)$/,
+            exclude: /node_modules/,
+            loader: '../chipper/node_modules/babel-loader',
+            options: {
+              presets: [
+                '../chipper/node_modules/@babel/preset-env',
+                {
+                  plugins: [
+                    '../chipper/node_modules/@babel/plugin-proposal-class-properties'
+                  ]
+                }
+              ]
+            }
+          }
+        ],
+      }
     } );
 
     compiler.run( ( err, stats ) => {

[samreid]

@jonathanolson and I looked into this further and found that the webpack babel did the transpilation, and the downstream transpilation wasn't doing anything further. We were confused by this, since the "browsers" list we specified in the downstream transpile was the same output as not specifying browsers in the webpack babel-loader.

We also found that this is not yet supported on Safari, perhaps it would be prudent to wait until there is better support there so we don't have to build to run on Safari? Though @jonathanolson said he would be OK with requiring builds to run on Safari.
https://caniuse.com/?search=static%20class%20fields

[pixelzoom]

We also found that this is not yet supported on Safari, perhaps it would be prudent to wait until there is better support there so we don't have to build to run on Safari?

Does this include mobile Safari? I'm pretty used to making a code change on my MacBook (lydian), and simply pointing my iPad to http://lydian.local/~cmalley/GitHub/ to quickly test changes. And I do that frequently during performance optimization phases of development. If I have to build every time I want to test a change, that's going to be costly.

[zepumph]

And I do that frequently during performance optimization phases of development. If I have to build every time I want to test a change, that's going to be costly.

I do that too for testing on my iPad. I feel like this would be a dealbreaker (especially since we just got past this issue for IE).

[samreid]

For convenience, here is a picture of the link that was linked above:

Note that neither Safari nor iOS has this feature yet. Perhaps it would be prudent to wait until there is better support there so we don't have to build to run on Safari.

[zepumph]

From dev meeting last week. We decided:

• We feel that we want to support unbuilt on safari
• We will set a calendar update to see when support for static fields gets added Safari. I'll add a calendar to check up on this in 6 months.
• For static getters, I will remove the lint rule and we can start using them. Dev discretion for when using this vs assignment after the class is made.

[zepumph]

in #983 (comment) @jonathanolson said:

It seems useful to have static get, AND it doesn't seem like we're transpiling classes now. Can we have this? Display's get focus turns into a static get when moving it to class.

I'll remove the lint rule now.

[zepumph]

Thanks for the ping.

[zepumph]

Removed in the above commit. I set a calendar reminded for April 29, 2021. Unassigning and deferring now.

[samreid]

https://caniuse.com/?search=static%20class%20fields reports that Safari 14.1 (released April 25, 2021) can now use static class fields. How will we decide when enough of our clients have upgraded to Safari 14.1? Also, I recommend we proceed with typescript as part of #987, which already supports static class members: https://www.typescriptlang.org/docs/handbook/2/classes.html#static-members

[chrisklus]

from dev meeting 5/6/20:

MK: should we get stats on who is using 14.1?

SR: should we pick a threshold for what is a good enough percentage of 14.1 users to use this feature

We looked at stats and very few users are up to 14.1! So we are going to add a calendar reminder about this.

[samreid]

Let's cover this with Typescript in phetsims/chipper#1050

[samreid]

I removed the calendar entry to investigate this, since it seems TypeScript will be a better solution. If we abandon TypeScript, we can reopen this issue.

[zepumph]

I have two feelings about this.

1. We should close this issue because typescript is great.
2. We should note that currently there is now complete support for this in caniuse.

Since Typescript features would largely be in "type space", it could be nice to note static fields as a possibility for "value space", I would still be interested in bringing this feature into our Typescript and our Javascript implementations. @samreid could we at least take steps to remove static field prevention from our project (I can't remember if there is a lint rule for it)?

[samreid]

We should note that currently there is now complete support for this in caniuse.

Our supported platforms says Safari 13+, but caniuse says this feature is unsupported through 14.

Since Typescript features would largely be in "type space", it could be nice to note static fields as a possibility for "value space",

This TypeScript code:

class V{
  static age:number=123;
}

compiles into this JavaScript code (according to the TS playground: https://www.typescriptlang.org/play?#code/FDDGBsEMGdoAgGoG9hztALpDBLUdIBzAUwC4A7AVwFsAjYgJwF4BGAJgGYBuYAX2CA

"use strict";
class V {
}
V.age = 123;

When I try a static class field in JS, I see this parse error:

Perhaps ESLint needs an upgraded parser to support that syntax?

That same code in a *.ts file has no syntax errors.

@zepumph where do you want to go with this issue next?

[zepumph]

I guess just close it. The fact that our eslint doesn't support it means that no one will be using it anytime soon. If someone wants to and feels strongly, then they can reopen or create a new issue. Likely TS is the way of the future. Thanks.

compiles into this JavaScript code (according to the TS playground: https://www.typescriptlang.org/play?#code/FDDGBsEMGdoAgGoG9hztALpDBLUdIBzAUwC4A7AVwFsAjYgJwF4BGAJgGYBuYAX2CA

I wonder/bet that this will change since static fields are recent additions to javascript's base language. Maybe sometime soon it will compile into static fields.