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

Update Class Fields to Stage 3 and change default behavior #6076

Merged
merged 4 commits into from Aug 10, 2017

Conversation

Projects
None yet
5 participants
@kedromelon
Contributor

kedromelon commented Aug 9, 2017

Q A
Patch: Bug Fix?
Major: Breaking Change? ✔️ (change to transform option)
Minor: New Feature?
Deprecations?
Spec Compliancy? ✔️
Tests Added/Pass?
Fixed Tickets Fixes #6073
License MIT
Doc PR
Dependency Changes
  • moves transform-class-properties from stage 2 to 3.
  • changes behavior to use spec mode by default, as requested by @hzoo in #6073 (please let me know if i misunderstood any part of that!)
  • updates tests
  • updates readme
@kedromelon

This comment has been minimized.

Contributor

kedromelon commented Aug 9, 2017

i added the {"loose": true} option to tests that were previously defaulting to non-spec mode. would it be better to update those tests' expected.js to use spec mode's output instead?

`boolean`, defaults to `false`.
Class properties are compiled to use `Object.defineProperty`. Static fields are now defined even if they are not initialized.
Class properties are compiled use an assignment expression instead of `Object.defineProperty`. Static fields are now not defined if they are not initialized.

This comment has been minimized.

@kedromelon

kedromelon Aug 9, 2017

Contributor

not sure how confident i am of the language here. does this capture the intended behavior?

This comment has been minimized.

@hzoo

hzoo Aug 9, 2017

Member

We can put the code example here to be more clear.

so an example of the defineProperty vs. this.x = a and also this.x = undefined

This comment has been minimized.

@kedromelon

kedromelon Aug 10, 2017

Contributor

added an example, though it may be a bit lengthy. let me know if you think we can trim it down a bit!

@@ -85,9 +85,9 @@ export default function({ types: t }) {
const propNode = prop.node;
if (propNode.decorators && propNode.decorators.length > 0) continue;
// In non-spec mode, all properties without values are ignored.
// In loose mode, all properties without values are ignored.
// In spec mode, *static* properties without values are still defined (see below).

This comment has been minimized.

@Kovensky

Kovensky Aug 9, 2017

Member

IIUC, in spec mode, all properties should be defined. This may also be a new change on the stage-3 version.

This comment has been minimized.

@kedromelon

kedromelon Aug 9, 2017

Contributor

yep, looks like it to me too! i'll change that here.

This comment has been minimized.

@kedromelon

kedromelon Aug 10, 2017

Contributor

think i covered these now. if you've got a chance to take another look, let me know if anything is missing!

@@ -36,7 +36,7 @@ export default function({ types: t }) {
VALUE: value ? value : t.identifier("undefined"),

This comment has been minimized.

@Kovensky

Kovensky Aug 9, 2017

Member

Probably better to use t.unaryExpression('void', t.numericLiteral(0)) to avoid any potential shadowing issues (fix out of scope maybe?)

This comment has been minimized.

@hzoo

hzoo Aug 9, 2017

Member

we have a method for it path.scope.buildUndefinedNode( - btw this should be a linting rule in babel/run against babel?

This comment has been minimized.

@kedromelon

kedromelon Aug 9, 2017

Contributor

👍 i'll replace with buildUndefinedNode here

This comment has been minimized.

@kedromelon

kedromelon Aug 9, 2017

Contributor

@hzoo regarding linting, would the rule be to always disallow t.identifier("undefined") in favor of path.scope.buildUndefinedNode()?

This comment has been minimized.

@hzoo

hzoo Aug 9, 2017

Member

I think so at least for us (that can be another pr, can do an inline plugin in the babelrc.js if we wanted), not sure the best way

This comment has been minimized.

@kedromelon

kedromelon Aug 9, 2017

Contributor

gotcha. cool, maybe i'll take a swing at that after this PR

`boolean`, defaults to `false`.
Class properties are compiled to use `Object.defineProperty`. Static fields are now defined even if they are not initialized.
Class properties are compiled use an assignment expression instead of `Object.defineProperty`. Static fields are now not defined if they are not initialized.

This comment has been minimized.

@jridgewell

jridgewell Aug 9, 2017

Member

Why are static fields not initialized? They should be undefined, too.

This comment has been minimized.

@hzoo

hzoo Aug 9, 2017

Member

i think it was never added for the default mode (which is now changing to loose)

This comment has been minimized.

@kedromelon

kedromelon Aug 9, 2017

Contributor

yeah, i just kept the behavior that existed as default for the new "loose" mode -- should this be changed?

This comment has been minimized.

@jridgewell

jridgewell Aug 9, 2017

Member

Yes.

update readme with examples; use `buildUndefinedNode()`; change behav…
…ior to always define both static and nonstatic class properties regardless of spec/loose mode; update tests

@hzoo hzoo added the Priority: High label Aug 10, 2017

@hzoo

This comment has been minimized.

Member

hzoo commented Aug 10, 2017

Amazing work @kedromelon!

@kedromelon

This comment has been minimized.

Contributor

kedromelon commented Aug 10, 2017

😄 thanks for the help everyone!

@hzoo

This comment has been minimized.

Member

hzoo commented Aug 10, 2017

Was a pretty big refactor! Also unforunately all the PRs will conflict since we are changing 3 different plugins to stage 2/3

@hzoo hzoo merged commit 4fdd756 into babel:7.0 Aug 10, 2017

0 of 2 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@hzoo

This comment has been minimized.

Member

hzoo commented Aug 10, 2017

@kedromelon also I noticed you were branching off of 7.0? I would suggest making a new branch for each PR in case you have multiple, and easier to manage

@kedromelon

This comment has been minimized.

Contributor

kedromelon commented Aug 10, 2017

@hzoo yeah, i probably will do that in the future! just didn't bother yet since i haven't had more than one PR going

@hzoo hzoo referenced this pull request Sep 12, 2017

Closed

July 2017 #19

10 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment