Stage 3 reviews

[littledan]

Thanks to everyone who signed up to do a Stage 3 review of decorators for the March TC39 meeting. Filing bugs in this repository is a great way to give feedback.

If it would be useful for any of you, I'd be happy to schedule a call where we can go over the proposal as a small group and we can discuss issues that way. Just let me know.

If you have reviewed this proposal and want to sign off on it as is, please say so on this thread and I'll check the box.

• @erights
• @waldemarhorwat
• @bradleymeck
• @dherman

Editors:

• @ljharb
• @zenparsing

[RWOverdijk]

Are these reviews needed for there to be progress on this proposal?

[ljharb]

@RWOverdijk yes, see the process document

[RWOverdijk]

Ah thanks. Anything I can do to help? I've been following decorators for a while now and I'd like to see it get approved. :)

[littledan]

@RWOverdijk Do you have any concerns with the current state of this proposal? If so, please file issues about them. If you can read the specification text closely to identify any possible problems, that will be a big help.

[RWOverdijk]

@littledan I do not, no. The proposal looks good imo. The only thing I have slight confusion about is how I would store metadata for a field (property) based on the class reference (so not per instance). Before I used to get the target (example decorator I once wrote here) and now it's a meta object to define the field, and I can't see a target being documented.

Other than that, I dislike the syntax (#) for private. But that's out of scope.

TL;DR; LGTM 👍

[trotyl]

@RWOverdijk You can do whatever to a class in the finisher.

[RWOverdijk]

Of course... You can return a finisher for a field. I mis-read this line:

Note: finishers run once per class (at class creation time), not once per instance.

Thanks. Then no, zero remarks.

[bmeck]

I have gone over the proposal and still have concerns with patterns around privacy and writing robust code, but do not see that as directly needing to block this proposal for stage 3 after the effort @littledan and @rbuckton have done to try and find alternatives. I will defer those requirements and find alternative ways of solving for them that are more universally applicable.

Reading the spec text, I do not have any remaining issues for stage 3 and feel it can advance.

[waldemarhorwat]

I reviewed the grammar implications of it. It looks good, although I wonder about the pros and cons of allowing yield and await inside the decorators in front of a class, which kinda depends on whether those decorators are logically part of the class. See the issue I filed.

[ljharb]

Filed a PR with some tweaks I found during review, otherwise LGTM from an editor's standpoint.

I still have personal concerns; primarily that I think decorators should go after export, and (much less after talking to @littledan) about robustness around leaks when decorating private fields.

[erights]

All my filed bugs except for #106 are minor. #106 is my only reservation on LGTMing this.

[pwhissell]

@littledan If I understood correctly, issue #106 seems to have been discussed and approved of. Where are we now in respect to promoting this proposal to Stage 3?

[littledan]

@pwhissell This proposal is on the agenda for the July 2018 TC39 meeting for consideration for Stage 3.

[pwhissell]

Thank you I think your link is broken it should probably be https://github.com/tc39/agendas/blob/master/2018/07.md
I didn't even know the tc39/agendas page existed. I love tc39 you guys are the best

[littledan]

@pwhissell Thanks, fixed. Yeah, check out the past agendas to see our awesome slide decks!

[ffMathy]

What happened at the meeting? What is the progress on stage 3?

[david0178418]

Looks like this week's meeting outlines some of the concerns. However, I'd really like to read some of the more detailed conversations that lead to the "blocker" label being applied.
https://docs.google.com/presentation/d/1s9bu_Z0vWR9eR4TL_8LEOmIFZvPth9Z8BLcHVqYWf_0/edit#slide=id.g42c5be6639_3_0

[ljharb]

@david0178418 all assigned stage 3 reviewers need to sign off before the proposal can reach stage 3; so this issue is a stage 3 blocker, and there's two boxes unchecked.

I suspect that doesn't answer your overall question, but it should explain why the process requires that this github issue be a stage 3 blocker.

[david0178418]

That definitely makes sense. However, the last meeting had "Decorators fortowards[sic] Stage 3" proposed, but that was clearly shot down (leading to @ffMathy 's question).

The slides I linked give some insight into the concerns, but it'd be more valuable to see some of the actual discussion that may have happened around this since it's such a big feature.

[littledan]

At this point, the main remaining unresolved issue is the ordering of decorators and export.

[ffMathy]

Thanks @littledan, but can you be more specific?

What is it about those areas that are not optimal?

[littledan]

@ffMathy Some people really want decorators before export, and others really want export before decorators.

I think it'd be OK either way; I don't see a fatal flaw with either option. My impression is that it's more important to standardize decorators than to get this detail "right" one way or the other.

TC39 is a consensus-based body, so the strongly opinionated parties will have to come to an agreement for decorators to move forwards.

[nicolo-ribaudo]

Hopefully https://github.com/search?utf8=✓&q="decoratorsBeforeExport%3A+true"&type=Code and https://github.com/search?utf8=✓&q="decoratorsBeforeExport%3A+false"&type=Code will give us meaningful stats about Babel's decoratorsBeforeExport usage before next TC39 meeting.

[jsg2021]

I would be careful about using those queries. I would bet the vast majority of current usages are either legacy or private.

[jsg2021]

The decorators before export group is probably a silent majority. I base this off of conversations over the past year or so when discussing this proposal with other developers and everyone has expressed shock/disapproval when I reveal export @dec class {} will probably be the spec. (This typically comes up when someone starts playing with decorators and gets excited and shares it with me...)

I'm doing my best not to influence initial instinct on placement... I have not yet seen someone write by instinct export @dec class {}, they always start with:

@dec
export class {}

They never inline it. They always put it on its own line.

[david0178418]

Is the preference completely driven by people's preference for
export class...

vs

export
class ...

If that's the case, this would be a terrible case of bike-shedding.

[rbuckton]

I am deeply concerned that export @dec class will quickly become a wart on the language the moment we add another modifier to class like abstract (i.e. export @dec abstract class) as it will be a source of confusion for developers.

[RWOverdijk]

I see the problem, and I do think both cases make sense. Decorators can't apply to exports, so they always apply to classes when placed above them. what's wrong with supporting both? If I had to choose, I'd say before export, simply because I've never written a decorator inline like that, especially because I sometimes use multiple. So +1 on @dec export class as mentioned by @jsg2021.

On the other hand, semantically it's weird. You're decorating a class that's being exported, rather than exporting a decorated class. So also a +1 for adding it after export. This would be prettier if you could export classes as named further down the file.

If my voice mattered, and I had to choose, I'd say before export.

[jsg2021]

@RWOverdijk you can export classes later in the file.

class Foo {...}

//As named
export {
 Foo
}
//or as default
export default Foo;

[david0178418]

Exporting at the end of the file is what I'm opting for in my code for the time being while all this is being worked out.

For what it's worth, I think logical consistency should win here and the decorators should follow exports.

[jsg2021]

@david0178418 logical consistency with what? We have two perspectives here. Some of us see the export keyword like return. Where others, like me, view it as an attribute/modifier... like static, abstract, etc...

[ljharb]

@jsg2021 that's the root of the disagreement for many; i believe export conceptually modifies the module, not the value - unlike static, abstract, etc.

[rbuckton]

[…] i believe export conceptually modifies the module, not the value - unlike static […]

Except that static modifies the containing class, not the method being decorated, similar to how export modifies the containing module, not the class being decorated**. While static and export do have their differences, they also share a number of similarities. This is one of many reasons export class feels very much like a modifier to me.

To me, export has never felt similar to return:

• return isn't statically analyzed:

  return { doesNotExist }; // throws at runtime
  // vs
  export { doesNotExist }; // throws statically

• return doesn't introduce a binding and doesn't hoist:

  console.log(foo); // throws ReferenceError: foo is not defined
  return function foo() {};
  // vs
  console.log(foo); // prints '[Function: foo]' in node
  export function foo() {}

• return effects statement evaluation:

  return class C {};
  console.log("hello"); // not executed
  // vs
  export class C {}
  console.log("hello"); // prints 'hello'

We also went to great lengths to differentiate export-related declarations from their expression counterparts:

return { foo, baz: bar };
// vs
export { foo, bar as baz };

The only similarity between the two is the odd corner case of export default since it evaluates an expression, but that's still a very weak similarity.

---

** As a side note, I do wonder how often modifying the placement of a static member will be used (especially due to [[HomeObject]] semantics), though we can't exactly restrict it.

[david0178418]

I think @ljharb hits at the heart of this. Modifiers like static modify the value vs the module.

Take the following:

@Foo
export
async function Bar() {}

In this case, Foo modifies Bar, export modifies the module, but then we go back to async modifying Bar.

However, by flipping the decorator and export, the value modifying keywords are grouped together.

edit: derp on the function decoration

[jsg2021]

@rbuckton is more convincing to me. static modifies the class, not the value.

I still struggle with deviation from other languages...

Java:

@somthing
public class Foo {
}

[david0178418]

Forgive the derp on the decorators on a function. I keep trying to do that in my code even though it's clearly not valid 🙃. I think it comes from the fact that it's just syntactic sugar for a function.

But I think that actually adds to why post-export makes sense. Since decorators are just functions that take in values and modify them, the export between the decorator and the class breaks that in my mind.

[littledan]

Let's continue this conversation about decorator/export ordering in #69 . This thread is for Stage 3 reviews.

[littledan]

Removed @gsathya as a reviewer at his request

[littledan]

There have been some edits of the specification following the previous Stage 3 reviews, so I would appreciate a further review ahead of the upcoming January 2019 TC39 meeting, if folks have time. To summarize the changelog and motivation:

• Ergonomics changes
  • Rationale: The new changes make decorators (subjectively) more straightforward to use. These changes all make sense (and many resolve counterintuitive things I noticed when I was writing code samples, but brushed off!), and as the proposal is at Stage 2, I want to be open to developer input for these sorts of superficial aspects.
  • Issue threads: Homonyms and confused names into descriptors #173 'kind' value is 'method' on decorator descriptor with get/set property definition #162 Change finishers to be normal element descriptors. #177
  • Unbox property descriptor in decorator descriptor Normative: Splat out the property descriptor in the decorator descriptor #198
  • Accessors are kind: "accessor", not method Normative: Name getter/setters as kind: "accessor" #200
  • Methods use method:, not value: Normative: Use method instead of value for the method #201
  • Initializers are kind: "hook", start() { Normative: Renamings in decorator descriptors #199
  • Finishers are a separate element, kind: "hook", finish() { or replace() { Normative: Renamings in decorator descriptors #199
• Decorator/private interaction
  • A PrivateName can only be "used" in a class element once, that is, multiple decorators may not expand into definitions in fields or methods of the same private name, but a decorator which changes a private field into a private method is permitted. This restriction may be revisited in a follow-on proposal, as it consists of throwing an error during class definition.
  • Rationale: See Normative: Disallow repeat use of private names #208
  • Issue: Restrict the interaction of decorators and private names to ensure limited overhead #180
  • PR: Normative: Disallow repeat use of private names #208
• Complete elements array
  • elements array previously contained only public (ordinary) fields and methods. However, this led to an unfortunate observable "sorting" of the order in which instance initializers run, that made identity decorators reorder the public field initializers before the private field ones, rather than interspersed. Including private class elements solves the issue, and it allows us to satisfy many important use cases that the previous restriction disallowed. Full rationale in Scoping down the use of PrivateName #133 (comment)
  • Issue: Scoping down the use of PrivateName #133
  • PR: Revert "Normative: Remove class decorator access to private names" #197
• Sloppy mode tweak
  • Class decorators are run in the mode of the surrounding context, not necessarily in strict mode (as previously)
  • Rationale: Potential future proposals for decorators (e.g., function, object) don't have to do with strict mode, and shouldn't invoke it, but we can't ask strict mode to look ahead to see if it's decorating a class. And this tweak is extremely minimal engineering effort to continue supporting sloppy mode.
  • Issue: Are decorators strict mode code? #204
  • PR: Sloppy decorator expressions #209 (Possible basis for a follow-on cleanup: Normative: Function.prototype.toString excludes class decorators #210 )

[littledan]

Some potentially controversial aspects of this proposal which need consensus for Stage 3 are highlighted at stage 3 blocker . In particular:

• Scoping down the use of PrivateName #133 "Scoping down the use of PrivateName": Various claims have been made that this proposal gives too much access of private names to decorators. I believe the exposure is justified by the mental model that a decorator is passed the class or class element that follows it, and this includes the contents of the class.
• Use case for changing "elements" in class decorators #186 "Use case for changing "elements" in class decorators": This proposal provides an "elements" array which contains all class elements, including private ones. The issue here explains why these use cases are essential for the decorators proposal, and why we have not removed access to class elements.

The use case involved in both of these issues here are essential for the proposal, and we've been designing the class fields and private methods proposals with these decorators capabilities in mind for several years, so I'd like to not reduce the power of decorators here.

[pwhissell]

what is the current status of decorators?

[littledan]

@pwhissell We've moved to a new model, but there are still several details to work out. See a description in README.md

[CICCIOSGAMINO]

When they'll land on stage 3 ?

[pzuraq]

Closing this is favor of #442, which is for the latest iteration of this proposal.