Skip to content
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

accessor keyword vs. { get; set; } #416

Closed
domenic opened this issue Jul 26, 2021 · 21 comments
Closed

accessor keyword vs. { get; set; } #416

domenic opened this issue Jul 26, 2021 · 21 comments

Comments

@domenic
Copy link
Member

domenic commented Jul 26, 2021

(I feel like I brought this up before but I can't find it... please feel free to dupe if so.)

@rbuckton has his own auto-accessors proposal in https://github.com/rbuckton/proposal-grouped-and-auto-accessors . It uses the syntax x { get; set; }.

This syntax has a few advantages IMO over accessor x:

  • It allows only a getter (or only a setter, if you want)
  • It allows a private setter/public getter (or the reverse, I guess) via x { get; #set; }
  • It reuses existing syntax semi-keywords instead of introducing a new one
  • It allows decorating either the accessor as a whole (@dec x { get; set; }) or only one of the accessor functions (x { get; @instrumented set; })

I'd encourage this proposal to build on that one if possible!

@rbuckton
Copy link
Collaborator

IIRC, Waldemar raised a concern with my auto-accessors proposal indicating that it needed a syntactic opt-in, otherwise we would be carving out an entire syntactic space (i.e., Identifier {...}) that could prevent future exploration of syntax similar to static {}. We've mostly agreed that the keyword used here will carry over to my proposal:

accessor x;

accessor x { get; set; } // same as above

accessor x {
  get() {...}
  set(v) {...}
}

With auto-accessors like accessor x { get; set; } giving you multiple decoration targets (the entire declaration, the getter, and the setter).

I much preferred prop over accessor, but there are concerns over confusing terminology (i.e., data properties vs. accessor properties, getOwnPropertyNames, etc.).

@matthewrobb
Copy link

@rbuckton It's odd to me as a long-time mostly silent auditor of tc39 that an argument be made for this "syntactic opt-in" based on the idea of some potential future language feature no one can practically imagine. Static blocks are keyword prefixed not identifier prefixed. Personally I would love to see your initial proposal but perhaps it's more than just accessor information, could it not also include any field configuration?

class {
  x {
    enumerable = ...
    get() {...}
    set(v) {...}
  }
}

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 2, 2022

@matthewrobb I think it's reasonable, we can't really predict the future and it would be unfortunate to be limited by auto accessors. Static blocks are a great example, I would not have predicted they would be necessary prior to the proposal, but they're quite useful (in fact, it would have been impossible to polyfill this proposal in Babel without them). While I agree that the shorter syntax would be nicer, in practice it's something I and others have quickly gotten used to, especially with IDE support.

I think the enumerable syntax is compelling, this is something that decorators no longer solve for. If generalized, how would this look for methods, async methods, generator methods, and fields? I think it would only make sense to remove the accessor keyword if it could apply to all types of class elements and not just accessors.

@matthewrobb
Copy link

@pzuraq

I think it would only make sense to remove the accessor keyword if it could apply to all types of class elements and not just accessors.

Yeah why not?

class {
  x {
    value = 1
    writable = false
  }
  
  y {
    value() {
    
    }
  }
}

@ljharb
Copy link
Member

ljharb commented Mar 2, 2022

@pzuraq um, are you saying decorators now can't be transpiled unless the target engine has native static blocks?

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 2, 2022

@ljharb no, Babel also transpiles static blocks, it's just that the timing semantics would have required us to modify the class fields transform to get everything to work. It would still have been possible, just significantly more annoying.

@matthewrobb my concern would be how this collides with auto-accessors. For instance, the following would be a valid combination:

class {
  x {
    value = 1;
    get;
    set;
  }
}

But would this also be valid?

class {
  x {
    value() {}
    get;
    set;
  }
}

The syntax seems to imply these would be composable in any way, but certain combinations don't really make sense.

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 2, 2022

Also, adding the accessor keyword does not prevent us from exploring this direction in the future. That syntax space would still be open, and accessor x; could become sugar for x { get; set; }

@matthewrobb
Copy link

matthewrobb commented Mar 2, 2022

@pzuraq

The syntax seems to imply these would be composable in any way, but certain combinations don't really make sense.

It might not be useful but it would work exactly as you'd expect would it not?

Also, adding the accessor keyword does not prevent us from exploring this direction in the future.

I can get behind this point more than the others thus far for sure.

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 2, 2022

It might not be useful but it would work exactly as you'd expect would it not?

I'm not sure exactly how it would work though? Would value as a method be overwritten by get/set? Would it be order dependent? Would we just pick an arbitrary ordering, e.g. get and set always win?

@matthewrobb
Copy link

I think it's extremely fair to say that the presence of get or set in a property descriptor always supersedes value semantics. Currently you can't defineProperty with both anyway. Thinking about how it desugars is the exact semantics I would expect and endorse. The syntactic form of the value in the block determines the Class element kind of the property. The presence of a non-method definition value field always implies instance own defined properties.

class {
  a { value = 1; } // instance defined
  b { value() {} } // proto defined
  c { get() {} } // proto defined
  d { value = 1; get; set; } // instance defined
}

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 2, 2022

hmm, fair. To be clear, the last one would be both proto and instance, auto-accessors are defined on the proto and the value itself is defined on the instance.

I could imagine actually pulling the assignment for an auto-accessor out as well, similar to the original proposal. Then we could have the exact same constraints as defineProperty, you can only provide value or get/set within the block itself:

class {
  a { value = 1;  } // valid
  b { value() {} } // valid
  c { get() {} } // valid
  d { get; set; } = 123; // valid

  b { value() {}; get; } // invalid
  d { value = 123; get; set; } // invalid
}

@matthewrobb
Copy link

@pzuraq I think it would be BRILLIANT if the semantics of those blocks matched defineProperty exactly!

@matthewrobb
Copy link

@pzuraq In fact, with your last examples we could do this!

class {
  x { enumerable = false  } = 1
}

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 2, 2022

I think this is worth considering and will raise it with the decorators group, thank you for raising the issues @matthewrobb. I'm still planning on proposing decorators in its current form with the accessor keyword for stage 3 at the end of this month, given that including accessor won't block this syntax in the future, and decorators are blocked on having the functionality that accessor provides. Ideally, we would move forward with decorators in it's current form, and continue iterating on grouped and auto-accessors afterward, but it's possible that ordering will be reversed in committee.

@matthewrobb
Copy link

@pzuraq By all means do whatever is the shortest path to Stage 3 🙏 Thanks for your hard work!

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 2, 2022

Another thought, this could also potentially be done with another new keyword, like define or something like that:

class {
  define x { enumerable = false  } = 1;
}

I would be less happy about this syntax, but if there were pushback for the same reason as there was around auto-accessors (e.g. taking up the whole syntax space) it might be a way to compromise to get the functionality.

@matthewrobb
Copy link

Another thought I am having is how the static analysis enabled by this would allow typescript to infer things it currently cannot:

class T {
  x { writable = false  }: number = 1;
}

const t = new T()
t.x = 2 // x is readonly

@nmn
Copy link

nmn commented Oct 13, 2023

FWIW, I want to chime in with my vote against the “accessor” keyword. It’s a confusing keyword that adds unnecessary new syntax. Even if it’s sugar for {get; set}, I’m against the idea of adding the sugar and making the language needlessly complex.

@justinfagnani
Copy link

Without accessor decorating fields is needlessly verbose.

@ljharb
Copy link
Member

ljharb commented Oct 13, 2023

The proposal is also already stage 3, and is unlikely to change.

@pzuraq
Copy link
Collaborator

pzuraq commented Feb 2, 2024

I'm going to close this issue as the discussion has moved over to the grouped-and-auto-accessors proposal. As noted, the accessor keyword is going to remain part of this proposal and is unlikely to change.

@pzuraq pzuraq closed this as completed Feb 2, 2024
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

No branches or pull requests

7 participants