Skip to content
This repository has been archived by the owner on May 19, 2018. It is now read-only.

Class Fields (combined) - Stage 2 #540

Closed
1 of 5 tasks
hzoo opened this issue May 26, 2017 · 11 comments
Closed
1 of 5 tasks

Class Fields (combined) - Stage 2 #540

hzoo opened this issue May 26, 2017 · 11 comments

Comments

@hzoo
Copy link
Member

hzoo commented May 26, 2017

Updates on class-field proposals (both public and private)

Champions: @jeffmo (public class fields) @littledan (private + combined)
Spec Repo: https://github.com/littledan/proposal-class-fields/
Spec Text: https://littledan.github.io/proposal-class-fields/
Slides: https://drive.google.com/file/d/0B-TAClBGyqSxWHpyYmg2UnRHc28/view

Remember that proposals are just that; subject to change until Stage 4!

Examples

class C {
  static x = 1, #y, [a];
  z, #w = 2, [b];
  
  a() {
    this.z;
    this.#w++;
    #w; // #w is this.#w
  }
}

Parsing/ESTree

  • AST handled already in current classProperties and classPrivateProperties.
  • May need to rename though (new plugin name).
  • make sure no "private computed" parse
  • need to support comma separated properties.

Transform

  • combine the class-properties plugin + tbd private properties one

Contacts

@diervo
Copy link
Contributor

diervo commented May 26, 2017

Since the code for classBody is a bit messy, as part of the "unification" I want to clean all that up.

Moreover, for internal renaming I would love to have feedback on the following (mostly implementation details):

  1. Renaming classProperty and classPrivateProperty to: classField and classPrivateField
    • Same for the internal AST nodes (used in the plugins).
  2. Potentially unifying all to have just one plugin for both cases maybe classFields
  3. Revisit wether the property name inside is a string or an Identifier.
    • Transformer plugins look very odd since you will have to path.skip() to avoid the internal identifier, just look odd.

@hzoo Will try to work on all of this over the weekend.
//cc @loganfsmyth 3) is just for you :) here is an example on why I want to change it:

function privateStatePlugin () {
  return {
    visitor: {
      Program() {
      },
      ClassPrivateProperty(path) {
        // We want to rename this one
      },
      PrivateName(path) {
         // IMHO most of the times, you want to skip here
         // path.node.name == Identifier
         // otherwise you will get the Identifier several times, sometimes unexpectedly
         path.skip();       
      },
      Identifier(path) {
        //console.log('\n>>>>> ID ', path.node); // Nothing new here.
      }
    },
  };
}

@littledan
Copy link

Note that Waldemar Horwat pointed out an ambiguity in the grammar, that because we can have static fields named static, set or get, it takes some degree of lookahead to tell whether something is a field declaration or a method definition. From some simple tests, it looks like Babel already handles this sort of case correctly already; I just wanted to mention that this is an area that is likely to change.

@diervo
Copy link
Contributor

diervo commented May 28, 2017

@littledan not sure I'm following, what is likely to change?

@littledan
Copy link

Well, I'm not sure exactly, I have sent an email to Waldemar to clarify. Now that I look at it some more, I'm not sure what the ambiguity is.

@littledan
Copy link

I heard back from Waldemar; it seems the issue was just a typo in the proposal, where it rendered wrong. But declarations end in a semicolon (and ASI fixes it). This was a recently introduced error in the spec formatting, so no need to hold anything up!

@bounceme
Copy link

bounceme commented Jun 2, 2017

tc39/proposal-class-public-fields#63

@littledan Is this the answer to above question?

and what about props and methods named like in/instanceof etc

@diervo
Copy link
Contributor

diervo commented Jun 2, 2017

I will try to give it a run over the weekend.

@littledan
Copy link

@bounceme in and instanceof are allowed as both public and private field names, as any IdentifierName can be used as a public or private field name (though if you use get, set or static, you may need to place more semicolons than you would otherwise).

@loganfsmyth
Copy link
Member

@diervo Still catching up on things, but for your comment about Identifier, I wouldn't recommend ever traversing for Identifier on its own, without checking in what context it is used, so you'd either just traverse for PrivateName or traverse for Identifier and ensure that its parent node is PrivateName.

@dead-claudia
Copy link
Contributor

@diervo To add to @loganfsmyth's comment, you still have to be aware of context even now, because of existing MemberExpressions (where computed: false) and the like.

diervo pushed a commit to diervo/babylon that referenced this issue Jul 1, 2017
- Fixes babel#540
- Fixes location for publicFields (should not contain semi-colon)
- Added tests
diervo pushed a commit to diervo/babylon that referenced this issue Jul 1, 2017
- Fixes babel#540
- Fixes location for publicFields (should not contain semi-colon)
- Allow decorators for comma separated fields
- Added tests
diervo pushed a commit to diervo/babylon that referenced this issue Jul 1, 2017
- Fixes babel#540
- Fixes location for publicFields (should not contain semi-colon)
- Allow decorators for comma separated fields
- Added tests
@hzoo hzoo added the Has PR label Jul 2, 2017
diervo pushed a commit to diervo/babylon that referenced this issue Jul 2, 2017
- Fixes babel#540
- Allow decorators for comma separated fields
- Added tests
@babel-bot
Copy link

This issue has been moved to babel/babel#6692.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants