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

Class private properties #6120

Closed
wants to merge 21 commits into
base: master
from

Conversation

@jridgewell
Member

jridgewell commented Aug 16, 2017

Q                       A
Fixed Issues
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? Yes
Tests Added/Pass? Yes
Spec Compliancy? Yes
License MIT
Doc PR
Any Dependency Changes?

Initial work on Class Private Properties. Still waiting on Babylon support for Private Methods.

It's been a month since I did this, so there are merge conflicts. Need to review again, but at least this is the starting point.

@xtuc

This comment has been minimized.

Show comment
Hide comment
@xtuc

xtuc Aug 16, 2017

Member

Looks awesome, nice work @jridgewell.

I think we could change the error message here:
babel-plugin-transform-class-properties/src/index.js#L85

How can we help you with the merge conflicts?

Member

xtuc commented Aug 16, 2017

Looks awesome, nice work @jridgewell.

I think we could change the error message here:
babel-plugin-transform-class-properties/src/index.js#L85

How can we help you with the merge conflicts?

@jridgewell

This comment has been minimized.

Show comment
Hide comment
@jridgewell

jridgewell Aug 18, 2017

Member

Alright, merge conflicts look resolved to me.

Member

jridgewell commented Aug 18, 2017

Alright, merge conflicts look resolved to me.

@jridgewell

This comment has been minimized.

Show comment
Hide comment
@jridgewell

jridgewell Aug 18, 2017

Member

But of course tests fail because of babel-standalone. I can confirm locally that standalone's dependency on babel-loader, and loader's dependency on babel-core@6.x, is causing the issue. Manually symlinking packages/babel-core into loader's node_modules fixes it.

We may need to remove standalone until we can bring loader into the monorepo.

Member

jridgewell commented Aug 18, 2017

But of course tests fail because of babel-standalone. I can confirm locally that standalone's dependency on babel-loader, and loader's dependency on babel-core@6.x, is causing the issue. Manually symlinking packages/babel-core into loader's node_modules fixes it.

We may need to remove standalone until we can bring loader into the monorepo.

jridgewell added a commit to jridgewell/babel that referenced this pull request Aug 19, 2017

Remove babel-standalone
This reverts #6029 due to issues in #6120.

We need to first move `babel-loader` into the monorepo (and update its
dependencies on `babel-core`). Afterwards, we can re-add
`babel-standalone`.
@Daniel15

This comment has been minimized.

Show comment
Hide comment
@Daniel15

Daniel15 Aug 19, 2017

Member

I submitted #6137 to fix the babel-standalone build, and confirmed that it fixes the build of this PR:

Member

Daniel15 commented Aug 19, 2017

I submitted #6137 to fix the babel-standalone build, and confirmed that it fixes the build of this PR:

@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Aug 23, 2017

Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/4809/

Collaborator

babel-bot commented Aug 23, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/4809/

@jridgewell

This comment has been minimized.

Show comment
Hide comment
@jridgewell

jridgewell Aug 23, 2017

Member

Ready for review. /cc @diervo

Member

jridgewell commented Aug 23, 2017

Ready for review. /cc @diervo

@diervo

This comment has been minimized.

Show comment
Hide comment
@diervo

diervo Aug 23, 2017

Contributor

This is awesome! I will review it tomorrow since I have a long flight ahead of me :)

Contributor

diervo commented Aug 23, 2017

This is awesome! I will review it tomorrow since I have a long flight ahead of me :)

@Qantas94Heavy

This comment has been minimized.

Show comment
Hide comment
@Qantas94Heavy

Qantas94Heavy Aug 26, 2017

Member

Is it just me, or is the handling of postfix increment/decrement handled incorrectly?

class A {
  #foo = 1;
  bar = 1;
  incrementFoo() {
    return this.#foo++;
  }
  incrementBar() {
    return this.bar++;
  }
}

let a = new A;
console.log(a.incrementFoo() === 1);
console.log(a.incrementBar() === 1);

For me that returns false, true.

What I did to solve this issue in my branch was to create separate helper functions for the various increment/decrement functions to avoid this issue -- would that be appropriate here?

Member

Qantas94Heavy commented Aug 26, 2017

Is it just me, or is the handling of postfix increment/decrement handled incorrectly?

class A {
  #foo = 1;
  bar = 1;
  incrementFoo() {
    return this.#foo++;
  }
  incrementBar() {
    return this.bar++;
  }
}

let a = new A;
console.log(a.incrementFoo() === 1);
console.log(a.incrementBar() === 1);

For me that returns false, true.

What I did to solve this issue in my branch was to create separate helper functions for the various increment/decrement functions to avoid this issue -- would that be appropriate here?

@jridgewell

This comment has been minimized.

Show comment
Hide comment
@jridgewell

jridgewell Aug 26, 2017

Member

Yup, I screwed up postfix.

Member

jridgewell commented Aug 26, 2017

Yup, I screwed up postfix.

assign = t.binaryExpression(
operator.slice(0, -1),
replaceWith,

This comment has been minimized.

@Qantas94Heavy

Qantas94Heavy Aug 26, 2017

Member

Doesn't this need to be coerced to a number?

@Qantas94Heavy

Qantas94Heavy Aug 26, 2017

Member

Doesn't this need to be coerced to a number?

This comment has been minimized.

@jridgewell

jridgewell Aug 26, 2017

Member

Fixed.

@jridgewell

This comment has been minimized.

@littledan

littledan Dec 28, 2017

Don't you need to coerce in the postfix case as well (while still not coercing in AssignmentExpressions)?

@littledan

littledan Dec 28, 2017

Don't you need to coerce in the postfix case as well (while still not coercing in AssignmentExpressions)?

@@ -193,13 +480,20 @@ export default function({ types: t }) {
path.insertAfter(nodes);
},
PrivateName(path) {

This comment has been minimized.

@Qantas94Heavy

Qantas94Heavy Aug 26, 2017

Member

I think this is enforced within Babylon, but shouldn't be too much harm to leave in.

@Qantas94Heavy

Qantas94Heavy Aug 26, 2017

Member

I think this is enforced within Babylon, but shouldn't be too much harm to leave in.

This comment has been minimized.

@littledan

littledan Dec 28, 2017

It's also enforced in the check on line 94.

@littledan

littledan Dec 28, 2017

It's also enforced in the check on line 94.

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

@danez danez closed this Aug 31, 2017

@danez danez reopened this Aug 31, 2017

@danez danez changed the base branch from 7.0 to master Aug 31, 2017

@hzoo hzoo referenced this pull request Sep 14, 2017

Closed

July 2017 #19

10 of 10 tasks complete
helpers.classPrivateFieldKey = defineHelper(`
var id = 0;
export default function _classPrivateFieldKey(name) {
// Can we use a middle finger emoji?

This comment has been minimized.

@sergeysova
@sergeysova

@jridgewell jridgewell closed this Sep 20, 2017

@jridgewell jridgewell reopened this Sep 20, 2017

@hzoo hzoo referenced this pull request Sep 27, 2017

Merged

Create sept-26.md #36

2 of 9 tasks complete

@jridgewell jridgewell requested a review from littledan Sep 27, 2017

@streamich streamich referenced this pull request Nov 21, 2017

Merged

Feat/react component #15

@hzoo hzoo added the Priority: High label Nov 29, 2017

@littledan

Great work setting this up, and many many apologies for my extreme delay in doing this review. I like that you included loose mode and spec mode in the way you did, and the semantics seem basically correct except for a number of edge cases.

test262 has fairly complete tests for private fields. You can run them against Babel by using test262-harness with the --babelPresets flag; you'll need an underlying JS shell, which you can get from jsvu.

See #7111 ; I'm fine for this patch to land with the spec compliance issues fixed and for that to be a follow-on.

@@ -19,6 +19,20 @@ defineType("BindExpression", {
},
});
defineType("ClassPrivateProperty", {

This comment has been minimized.

@littledan

littledan Dec 28, 2017

Nit: rather than saying "private property", I've been sticking to the term "private field". Properties are public. Private fields are not properties. Private fields don't have the property introspection API that all properties support. Applies here for the type as well as the name of the feature. In general, I'd prefer if "class properties" were referred to as "class fields"--all classes have something to do with properties--though I understand if the name of the existing transform can't change at this point.

@littledan

littledan Dec 28, 2017

Nit: rather than saying "private property", I've been sticking to the term "private field". Properties are public. Private fields are not properties. Private fields don't have the property introspection API that all properties support. Applies here for the type as well as the name of the feature. In general, I'd prefer if "class properties" were referred to as "class fields"--all classes have something to do with properties--though I understand if the name of the existing transform can't change at this point.

}
return receiver;
}
`);

This comment has been minimized.

@littledan

littledan Dec 28, 2017

Looks like you use classPrivateFieldBase/classPrivateFieldKey in the loose mode transform, and classPrivateFieldGet/classPrivateFieldSet in the normal mode. Is that right? Consider adding comments or putting "loose" in the names of the first two to make this more locally apparent.

@littledan

littledan Dec 28, 2017

Looks like you use classPrivateFieldBase/classPrivateFieldKey in the loose mode transform, and classPrivateFieldGet/classPrivateFieldSet in the normal mode. Is that right? Consider adding comments or putting "loose" in the names of the first two to make this more locally apparent.

var _temp, _this;
babelHelpers.classCallCheck(this, B);
return babelHelpers.possibleConstructorReturn(_this, (_temp = _this = babelHelpers.possibleConstructorReturn(this, (B.__proto__ || Object.getPrototypeOf(B)).call(this, ...args)), Object.defineProperty(_this, "foo", {
return babelHelpers.possibleConstructorReturn(_this, (_temp = _this = babelHelpers.possibleConstructorReturn(this, (B.__proto__ || Object.getPrototypeOf(B)).apply(this, arguments)), Object.defineProperty(_this, "foo", {

This comment has been minimized.

@littledan

littledan Dec 28, 2017

As explained above, this is a regression.

@littledan

littledan Dec 28, 2017

As explained above, this is a regression.

newConstructor.body.body.push(
t.returnStatement(
t.callExpression(t.super(), [
t.spreadElement(t.identifier("args")),
t.spreadElement(t.identifier("arguments")),

This comment has been minimized.

@littledan

littledan Dec 28, 2017

I'm not sure what caused this change, but it's a regression in terms of pedantic correctness (even if it's appropriate for loose mode). In particular, programmers can monkey-patch Array.prototype[Symbol.iterator] and observe the ... in action. Could this have been a mistake in a rebase?

@littledan

littledan Dec 28, 2017

I'm not sure what caused this change, but it's a regression in terms of pedantic correctness (even if it's appropriate for loose mode). In particular, programmers can monkey-patch Array.prototype[Symbol.iterator] and observe the ... in action. Could this have been a mistake in a rebase?

This comment has been minimized.

@CodingItWrong

CodingItWrong Mar 20, 2018

Contributor

@jridgewell FYI I'm planning on rolling this change back on my PR; in working through the code I wasn't able to find a reason this change needs to be made. If you think I should leave it in just let me know.

@CodingItWrong

CodingItWrong Mar 20, 2018

Contributor

@jridgewell FYI I'm planning on rolling this change back on my PR; in working through the code I wasn't able to find a reason this change needs to be made. If you think I should leave it in just let me know.

Function(path) {
if (path.isArrowFunctionExpression()) return;
path.skip();
},

This comment has been minimized.

@littledan

littledan Dec 28, 2017

Does Babel support eval? If so, you might want to error out if eval is found nested inside a constructor when this plugin is turned on (since it will not be handled correctly--a super call may be nested in an eval), or maybe such behavior would be more trouble than it's worth.

@littledan

littledan Dec 28, 2017

Does Babel support eval? If so, you might want to error out if eval is found nested inside a constructor when this plugin is turned on (since it will not be handled correctly--a super call may be nested in an eval), or maybe such behavior would be more trouble than it's worth.

This comment has been minimized.

@xtuc

xtuc Mar 21, 2018

Member

Babel doesn't parse the content of the eval, it's maybe simpler for now to just throw an error.

@xtuc

xtuc Mar 21, 2018

Member

Babel doesn't parse the content of the eval, it's maybe simpler for now to just throw an error.

if (
grandParentPath.isAssignmentExpression() ||
grandParentPath.isUpdateExpression()

This comment has been minimized.

@littledan

littledan Dec 28, 2017

This seems like reasonable handling of this.#x = y and this.#x++, but I'm not sure how other LHS contexts are handled, such as

  [ this.#x ] = [1];
  this.#x += 1;

I see you have exec tests for the second case, but I can't understand how it works.

@littledan

littledan Dec 28, 2017

This seems like reasonable handling of this.#x = y and this.#x++, but I'm not sure how other LHS contexts are handled, such as

  [ this.#x ] = [1];
  this.#x += 1;

I see you have exec tests for the second case, but I can't understand how it works.

}
const call = t.clone(grandParentPath.node);
call.callee = t.memberExpression(replaceWith, t.identifier("call"));

This comment has been minimized.

@littledan

littledan Dec 28, 2017

Should there be any attempt to call this function with the original Function.prototype.call, rather than whatever the call property is of that function? (I'm not sure what Babel does here in general.)

@littledan

littledan Dec 28, 2017

Should there be any attempt to call this function with the original Function.prototype.call, rather than whatever the call property is of that function? (I'm not sure what Babel does here in general.)

if (parentPath.isClassPrivateProperty({ key: node })) return;
throw path.buildCodeFrameError(
`illegal syntax. Did you mean \`this.#${node.id.name}\`?`,

This comment has been minimized.

@littledan

littledan Dec 28, 2017

The error message here could refer to the fact that this is proposed as a follow-on, at https://github.com/littledan/proposal-private-shorthand . The committee has specifically discussed that it'd be great if Babel error messages for shorthand property usage to refer to this. It would be fine to make as a separate transform and include in the Stage 2 preset. cc @wycats

@littledan

littledan Dec 28, 2017

The error message here could refer to the fact that this is proposed as a follow-on, at https://github.com/littledan/proposal-private-shorthand . The committee has specifically discussed that it'd be great if Babel error messages for shorthand property usage to refer to this. It would be fine to make as a separate transform and include in the Stage 2 preset. cc @wycats

assign = t.binaryExpression(
operator.slice(0, -1),
replaceWith,

This comment has been minimized.

@littledan

littledan Dec 28, 2017

Don't you need to coerce in the postfix case as well (while still not coercing in AssignmentExpressions)?

@littledan

littledan Dec 28, 2017

Don't you need to coerce in the postfix case as well (while still not coercing in AssignmentExpressions)?

);
}
assign = t.binaryExpression(

This comment has been minimized.

@littledan

littledan Dec 28, 2017

Very cute that this path works for both UpdateExpressions and AssignmentExpressions (e.g., the way right is not undefined only if it's an AssignmentExpression), but the duality was a bit confusing to read. It might benefit from either a comment or splitting into two separate branches.

@littledan

littledan Dec 28, 2017

Very cute that this path works for both UpdateExpressions and AssignmentExpressions (e.g., the way right is not undefined only if it's an AssignmentExpression), but the duality was a bit confusing to read. It might benefit from either a comment or splitting into two separate branches.

@billinghamj

This comment has been minimized.

Show comment
Hide comment
@billinghamj

billinghamj Jan 13, 2018

Quick weird example to consider:

class A {
  #a;
  
  foo() {
    const self = this;
    
    // works
    console.log(self.#a);
    
    class B {
      bar() {
      	// fails: unknown private property
        console.log(self.#a);
      }
    }
  }
}

Currently, this does not work - https://babeljs.io/repl/build/4809/

Should it? I think probably yes, but I don't have complete understanding of the spec. If yes, this is a bug.

billinghamj commented Jan 13, 2018

Quick weird example to consider:

class A {
  #a;
  
  foo() {
    const self = this;
    
    // works
    console.log(self.#a);
    
    class B {
      bar() {
      	// fails: unknown private property
        console.log(self.#a);
      }
    }
  }
}

Currently, this does not work - https://babeljs.io/repl/build/4809/

Should it? I think probably yes, but I don't have complete understanding of the spec. If yes, this is a bug.

@TrySound

This comment has been minimized.

Show comment
Hide comment
@TrySound

TrySound Jan 13, 2018

Contributor

@billinghamj It shouldn't, I guess. Same as an access like const a = new A(); a.#field;

Contributor

TrySound commented Jan 13, 2018

@billinghamj It shouldn't, I guess. Same as an access like const a = new A(); a.#field;

@billinghamj

This comment has been minimized.

Show comment
Hide comment
@billinghamj

billinghamj Jan 13, 2018

@TrySound I'm not sure that's comparable. In my example, the second class is within the scope of the first class. So normally any closure rules would apply.

edit: I have asked the question: tc39/proposal-class-fields#71

billinghamj commented Jan 13, 2018

@TrySound I'm not sure that's comparable. In my example, the second class is within the scope of the first class. So normally any closure rules would apply.

edit: I have asked the question: tc39/proposal-class-fields#71

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Jan 13, 2018

I agree, that example should work. I believe I pointed out the issue on line 97 of the transform.

littledan commented Jan 13, 2018

I agree, that example should work. I believe I pointed out the issue on line 97 of the transform.

@littledan littledan referenced this pull request Mar 6, 2018

Open

Class Fields (Stage 3) #12

2 of 5 tasks complete
@diervo

This comment has been minimized.

Show comment
Hide comment
@diervo

diervo Mar 6, 2018

Contributor

@jridgewell @littledan Next week I will have sometime to dedicate to work on this since it will be a prerequisite for decorators. Was there any remaining issues with this PR?

Among other things, I'm planning to rename everything 'property' to field so we can start from a clean slate.

Contributor

diervo commented Mar 6, 2018

@jridgewell @littledan Next week I will have sometime to dedicate to work on this since it will be a prerequisite for decorators. Was there any remaining issues with this PR?

Among other things, I'm planning to rename everything 'property' to field so we can start from a clean slate.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Mar 6, 2018

@diervo If you look in the files tab, I've made a number of comments on this PR. I don't think they've been addressed. The biggest correctness issues are in the function collectPropertiesVisitor.

littledan commented Mar 6, 2018

@diervo If you look in the files tab, I've made a number of comments on this PR. I don't think they've been addressed. The biggest correctness issues are in the function collectPropertiesVisitor.

@diervo

This comment has been minimized.

Show comment
Hide comment
@diervo

diervo Mar 7, 2018

Contributor

Cool, will get a stab at getting it to the finish line this week.

Contributor

diervo commented Mar 7, 2018

Cool, will get a stab at getting it to the finish line this week.

@isiahmeadows

This comment has been minimized.

Show comment
Hide comment
@isiahmeadows
Contributor

isiahmeadows commented Mar 8, 2018

@billinghamj

This comment has been minimized.

Show comment
Hide comment
@billinghamj

billinghamj Mar 13, 2018

Looks like this has been replaced by #7555

billinghamj commented Mar 13, 2018

Looks like this has been replaced by #7555

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Mar 30, 2018

Looks like this work is continuing in #7555 . Should this PR be closed?

littledan commented Mar 30, 2018

Looks like this work is continuing in #7555 . Should this PR be closed?

@jridgewell jridgewell closed this Apr 4, 2018

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