Greater spec compliance for class properties #4544

motiz88 · 2016-09-22T12:55:53Z

Q	A
Bug fix?	yes
Breaking change?	possibly
New feature?	no
Deprecations?	no
Spec compliancy?	yes
Tests added/pass?	yes
Fixed tickets	#4463
License	MIT

Class property definitions are now transpiled to Object.defineProperty calls, following the latest proposed spec for instance fields and static fields.
Static fields are now defined even if they are not initialized. This curious difference between statics and instance fields is in the spec (see above links).
This now includes computed property support (Computed class properties #4500).

Oddities

In a couple of places (see commit comments), function name inference apparently kicks in on the intermediate code, e.g. turning Object.defineProperty(..., {value: function() {}}) into ... value: function value() {} ..... This may not be desired.

Future work

Collect consecutive definitions of the same kind into Object.defineProperties calls?

motiz88 · 2016-09-22T12:58:27Z

packages/babel-plugin-transform-class-properties/test/fixtures/general/foobar/expected.js

+        Object.defineProperty(_this, "scopedFunctionWithThis", {
+            enumerable: true,
+            writable: true,
+            value: function value() {


value: function value() ^^^^^

Kinda meaningless function name inference here.

Hmm, yeah seems like we'd ideally want to implement name inference here better, though it doesn't appear to be officially in the class-properties spec text anyway: tc39/proposal-class-public-fields#48

motiz88 · 2016-09-22T13:02:33Z

packages/babel-plugin-transform-class-properties/test/fixtures/regression/T7364/expected.js

+          console.log(_this);
+        });
+
+        return function value() {


return function value() { ^^^^^

Kinda meaningless function name inference here. Also, the surrounding IIFE's sole purpose is to rename the function returned from asyncToGenerator.

codecov-io · 2016-09-22T13:03:47Z

Current coverage is 88.83% (diff: 100%)

No coverage report found for master at 2dc919d.

Powered by Codecov. Last update 2dc919d...71da4ad

loganfsmyth · 2016-09-22T15:55:11Z

Commented on #4463 (comment) but duplicating over here:

I think the fact that the Node REPL disagrees with the spec doesn't necessarily make this an obvious non-breaking change. I totally agree we should land this change, but since at least according to the spec it has the potential to be a breaking change, I'd prefer if we perhaps initially landed it as an opt-in option for the class property transform or something.

As-is, this will also break transform-decorators-legacy unfortunately, so that will need to be fixed to detect both cases, I think.

hzoo · 2016-09-22T15:56:01Z

We could do a loose mode for class properties as well? (although since it's breaking it would be the opposite atm)

loganfsmyth · 2016-09-22T16:30:38Z

Yeah, perhaps we should go with spec: true to get this behavior?

loganfsmyth · 2016-09-22T16:58:42Z

I think the fact that the Node REPL disagrees with the spec doesn't necessarily make this an obvious non-breaking change. I totally agree we should land this change, but since at least according to the spec it has the potential to be a breaking change, I'd prefer if we perhaps initially landed it as an opt-in option for the class property transform or something.

Nevermind, I just didn't read the spec right :P We should be good as far as Object.defineProperty goes.

I'm all for this, but I do think it will need to be opt-in unfortunately, until we bump our major version anyway.

hzoo · 2016-09-22T17:34:01Z

Yeah, perhaps we should go with spec: true to get this behavior?

Ok 👍

looks something like https://github.com/babel/babel/blob/master/packages/babel-plugin-transform-es2015-arrow-functions/src/index.js#L4-L5 https://github.com/babel/babel/blob/master/packages/babel-plugin-transform-es2015-arrow-functions/README.md

motiz88 · 2016-09-22T23:09:50Z

Alrighty. The two new behaviors in this PR (defineProperty all the properties, and statics are always defined) are now conditional upon spec: true.

loganfsmyth · 2016-09-25T20:48:24Z

packages/babel-plugin-transform-class-properties/src/index.js

@@ -1,5 +1,5 @@
 /* eslint max-len: 0 */
-// todo: define instead of assign
+


Can we drop this extra newline?

loganfsmyth · 2016-09-25T20:53:45Z

packages/babel-plugin-transform-class-properties/src/index.js

+    loose: (ref, {key, value}) => t.expressionStatement(
+      t.assignmentExpression("=", t.memberExpression(ref, key), value) // TODO(motiz88): handle computed key
+    )
+  };


We'd be better off using babel-template for these, e.g.

import templates from 'babel-templates'; const buildPropertySpec = template(` Object.defineProperty(REF, KEY, { // configurable is false by default enumerable: true, writable: true, value: VALUE, }); `); const buildProperty = template(` REF[KEY] = VALUE; `);

And per my example, let's not call the old behavior "loose" since it's not technically "loose" mode and that might lead to confusion.

You can also use template from the babel parameter - export default function ({ types: t, template }) { but it would be created inside the function

That works too, though I think most of our plugins import it.

I've now implemented this change, but kept buildProperty as a "raw" AST expression due to logic surrounding computed properties I'm not sure how to efficiently pack into template form.

loganfsmyth · 2016-09-25T20:55:16Z

packages/babel-plugin-transform-class-properties/src/index.js

@@ -55,18 +79,16 @@ export default function ({ types: t }) {
        for (let prop of props) {
          let propNode = prop.node;
          if (propNode.decorators && propNode.decorators.length > 0) continue;
-          if (!propNode.value) continue;
+
+          if (!state.opts.spec && !propNode.value) continue;


Feels like this could use a comment to make it clear why this is done.

motiz88 · 2016-09-26T21:55:57Z

I think this is it, as far as the requested changes go.

I'm not crazy about the names buildDefineProperty, buildPropertySpec, buildProperty, buildPropertyDefinition, though. It's late and my brain can't come up with a reasoned naming scheme for these right now, so I'll be taking ideas if anyone has them.

into spec-class-properties

motiz88 · 2016-10-16T13:20:14Z

Gave the helper functions new names, updated to latest master, everything works! 😀 Are we good to go? @loganfsmyth

* Desugar class properties to Object.defineProperty per spec * Define uninitialized static class properties with value=undefined * Make class-properties increased spec compliance opt-in (spec: true)

motiz88 commented Sep 22, 2016

View reviewed changes

danez added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Sep 22, 2016

motiz88 commented Sep 22, 2016

View reviewed changes

loganfsmyth requested changes Sep 25, 2016

View reviewed changes

motiz88 added 5 commits September 27, 2016 00:06

DRY up property init in transform-class-properties

1608dca

Desugar class properties to Object.defineProperty per spec

2076878

Define uninitialized static class properties with value=undefined

f58d246

Make class-properties increased spec compliance opt-in (spec: true)

71bd9b8

Revert es2015-parameters test to use class-properties with {spec: false}

935d7e8

motiz88 force-pushed the spec-class-properties branch from c0d0f5e to 935d7e8 Compare September 26, 2016 21:13

motiz88 added 3 commits September 27, 2016 00:29

Comments for clarity

659a0d1

Use babel-template for class-properties spec mode

6402161

Add babel-template dependency to transform-class-properties

a3f464b

motiz88 added 4 commits October 13, 2016 18:51

Merge branch 'master' into spec-class-properties

8a9323d

More sensible names for new functions

bcaae15

Merge branch 'spec-class-properties' of https://github.com/motiz88/babel

6c8fe51

into spec-class-properties

Merge branch 'master' into spec-class-properties

71da4ad

DrewML approved these changes Nov 15, 2016

View reviewed changes

[skip ci]

3acbf54

hzoo merged commit f8ddd80 into babel:master Nov 15, 2016

rhoggSugarcrm mentioned this pull request Jan 26, 2017

Upgrade babel sugarcrm/thorn#102

Merged

hzoo mentioned this pull request Jul 26, 2017

transform-class-properties: treatment of undefined properties (i.e. properties without default values) #5056

Closed

hzoo mentioned this pull request Sep 12, 2017

July 2017 babel/proposals#19

Closed

10 tasks

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 7, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Greater spec compliance for class properties #4544

Greater spec compliance for class properties #4544

motiz88 commented Sep 22, 2016 •

edited

Loading

motiz88 Sep 22, 2016

loganfsmyth Sep 22, 2016

motiz88 Sep 22, 2016 •

edited

Loading

codecov-io commented Sep 22, 2016 •

edited

Loading

loganfsmyth commented Sep 22, 2016

hzoo commented Sep 22, 2016

loganfsmyth commented Sep 22, 2016

loganfsmyth commented Sep 22, 2016

hzoo commented Sep 22, 2016

motiz88 commented Sep 22, 2016

loganfsmyth Sep 25, 2016

loganfsmyth Sep 25, 2016

loganfsmyth Sep 25, 2016

hzoo Sep 26, 2016

loganfsmyth Sep 26, 2016

motiz88 Sep 26, 2016

loganfsmyth Sep 25, 2016

motiz88 commented Sep 26, 2016

motiz88 commented Oct 16, 2016

		@@ -1,5 +1,5 @@
		/* eslint max-len: 0 */
		// todo: define instead of assign

Greater spec compliance for class properties #4544

Greater spec compliance for class properties #4544

Conversation

motiz88 commented Sep 22, 2016 • edited Loading

Oddities

Future work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

motiz88 Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Sep 22, 2016 • edited Loading

Current coverage is 88.83% (diff: 100%)

loganfsmyth commented Sep 22, 2016

hzoo commented Sep 22, 2016

loganfsmyth commented Sep 22, 2016

loganfsmyth commented Sep 22, 2016

hzoo commented Sep 22, 2016

motiz88 commented Sep 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

motiz88 commented Sep 26, 2016

motiz88 commented Oct 16, 2016

motiz88 commented Sep 22, 2016 •

edited

Loading

motiz88 Sep 22, 2016 •

edited

Loading

codecov-io commented Sep 22, 2016 •

edited

Loading