Skip to content

Baseline changes for ESNext Class Property Transformation #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 27, 2018

Conversation

joeywatts
Copy link
Owner

The transformation of class fields has been moved to the ESNext transformation on this branch.

This PR is for discussion on the baseline changes (so it doesn't crowd the logic of the PR).

@joeywatts joeywatts force-pushed the class-properties-esnext-baselines branch from 0126eba to a5cfa5a Compare September 21, 2018 18:18
@joeywatts joeywatts force-pushed the class-properties-esnext branch 4 times, most recently from 2c8ef42 to 5e9f170 Compare September 21, 2018 20:28
@joeywatts joeywatts force-pushed the class-properties-esnext-baselines branch from a5cfa5a to b4abb86 Compare September 21, 2018 20:36
@joeywatts joeywatts force-pushed the class-properties-esnext-baselines branch from b4abb86 to 1c5354c Compare September 24, 2018 21:03
@mheiber
Copy link

mheiber commented Sep 26, 2018

10:34:31 What we're looking for is making sure that the semantics don't change, with the exception of (I think) one intentional change: that when there is a field def, the property is added to the object (with value undefined if value isn't set)

(@joeywatts found this TS issue for passing through properties in this manner: microsoft#12212)

@mheiber
Copy link

mheiber commented Sep 26, 2018

note on how to review this:

The TS test cases work as follows:

  • Test inputs (called "cases") typically end in .ts. They are not included in this PR (since the inputs didn't change). Example: tests/cases/compiler/decoratorsOnComputedProperties.ts
  • Test case expected output (called "baselines") are all in tests/baselines/reference

So I'd recommend reviewing this by cloning the TS repo. Then, for each .js in the diff, go to the .ts file with the same name. See if the new baseline output has either:

  • the same semantics as the original
  • different semantics but in a way that is desirable

This is really difficult!

@@ -196,7 +196,8 @@ var __decorate = (this && this.__decorate) || function (decorators, target, key,
else for (var i = decorators.length - 1; i >= 0; i--) if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did this baseline change?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TypeScript transformer handles the experimental decorator transformation. This made pulling out the property transformation from the TypeScript transformer difficult because there are computed property names that have to be used in both the original property declaration/assignment and in the call to the decorator helper function (and non-trivial computed property name expressions shouldn't be executed more than once).

I reconciled this by transforming the computed property names (not the actual property declaration/initializer) in the TypeScript transformer when they need to be used more than once by code generated by the TypeScript transformer.

The _a, _b, _c, _d variables are now hoisted in the ESnext transformer (they are class aliases which are emitted when a static property initializer references the name of its own class).
The other hoisted variables store computed property names for the decorated properties (created by the TypeScript transformer).

static [y]() { return 0; }
static get [z]() { return 0; }
static set [w](value) { }
[x];
[y]() { return 0; }
get [z]() { return 0; }
set [w](value) { }
Copy link

@mheiber mheiber Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

intentional semantic change here

@@ -1,9 +1,7 @@
class C {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where are the source files for the transformApi tests?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They just live in this one file: src/testRunner/unittests/transform.ts

Copy link

@mheiber mheiber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving,

static readonlyStaticType;
static readonlyStaticTypeAndCall = Symbol();
static readwriteStaticCall = Symbol();
readonlyCall = Symbol();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did the space in the source file go away?

class C {
    static readonly readonlyStaticCall = Symbol();
    static readonly readonlyStaticType: unique symbol;
    static readonly readonlyStaticTypeAndCall: unique symbol = Symbol();
    static readwriteStaticCall = Symbol();

    readonly readonlyCall = Symbol();
    readwriteCall = Symbol();
}

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the emit never preserves the whitespace of the source file (from looking at the previous baseline and playing around with the TS playground)

Copy link

@mheiber mheiber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@joeywatts joeywatts merged commit 946fb65 into class-properties-esnext Sep 27, 2018
joeywatts added a commit that referenced this pull request Oct 5, 2018
* Add baselines

* Update baselines

Signed-off-by: Joseph Watts <jwatts43@bloomberg.net>
joeywatts pushed a commit that referenced this pull request Oct 18, 2018
Note that these annotations are parsed by the tool in Microsoft/vscode-gdpr-tooling; the associated PR #4 adds Typescript to what the tool processes.
joeywatts added a commit that referenced this pull request Oct 18, 2018
* Add baselines

* Update baselines

Signed-off-by: Joseph Watts <jwatts43@bloomberg.net>
joeywatts added a commit that referenced this pull request Oct 23, 2018
* Add baselines

* Update baselines

Signed-off-by: Joseph Watts <jwatts43@bloomberg.net>
joeywatts added a commit that referenced this pull request Nov 7, 2018
* Add baselines

* Update baselines

Signed-off-by: Joseph Watts <jwatts43@bloomberg.net>
joeywatts added a commit that referenced this pull request Nov 12, 2018
* Add baselines

* Update baselines

Signed-off-by: Joseph Watts <jwatts43@bloomberg.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants