tc39 / proposal-decorators

Decorators for ES6 classes
https://arai-a.github.io/ecma262-compare/?pr=2417
2.75k stars 106 forks source link

Static decorator declaration syntax #252

Closed littledan closed 4 years ago

littledan commented 5 years ago

In #250, the syntax

const @decorator = @(baz) => @foo @bar(baz);

is proposed. @trotyl proposed a different syntax which I like better, the more I think about it:

decorator @decorator(baz) {
  @foo @bar(baz)
}

I'm going to switch the draft explainer to use @trotyl's proposed syntax, but I'd still like to hear your suggestions about other alternatives, or thoughts on the tradeoffs.

rdking commented 5 years ago

Using a syntax like @trotyl's suggestion is what I've been hoping for. Even if it's still a function under the hood, it should not use the same keyword as a function since it is primarily for a different purpose.

Just 1 question though: is the @ 1) part of the name of the decorator, 2) an operator to trigger the use of the decorator, or 3) a literal to signify that "the following [[IdentifierName]] is a decorator"? If:

  1. then the syntax above is just almost as off-putting as the # from private fields. But that becomes a matter of taste, so I'll say nothing further regarding taste. However, it should mean that @ is now a valid character for [[IdentifierNames]]. I don't think that's within the scope of the change you're trying to make.
  2. then the syntax above feels more like an error, as you are trying to declare a decorator and not use one. The problem is the @ in the name here. It might be better specified as
    decorator myDecorator(baz) {
    @foo @bar(baz)
    }
  3. then the syntax above feels as redundant as specifying private #foo, an acceptable thing if the redundancy serves a technical purpose. This once again boils down to a matter of taste, and I'm not entirely certain how I feel on this one. So again, I won't argue over taste.
littledan commented 5 years ago
  1. @ is part of the name in this proposal.
rdking commented 5 years ago

So decorators will be following the somewhat unfortunate pattern of adding an otherwise invalid character to an identifier name specifically and only for use with decorators, much like the sigil with private names. Oh well. I just want the feature 😄, and the awkwardness doesn't pose any major issues.

So yeah, I prefer the new syntax. It feels more natural than the previous. Do you already have a thread for discussing the details of your new proposal?

littledan commented 5 years ago

Most of the people I've talked to are really positive about @ being part of the name. Most of the documentation for decorators that I found already treats it like it's part of the name.

There are some threads to discuss some details (#248, #251, #252), but feel free to open a new one if you see a topic that's missing.

rdking commented 5 years ago

Ok. I opened #253 as I didn't think this fit well in the other threads.

nicolo-ribaudo commented 5 years ago

I think that allowing other statements to be put there would be really useful, for example to store weakmaps.

Would this be allowed?

decorator @decorator(v) {
  let a = 1;
  @foo @bar(v + a)
}

Also, by looking at it I feel like the "implicit return" isn't the best thing, but using it explicitly is worst since it looks like it could be somewhere else rather then in the last position.

decorator @decorator(v) {
  let a = 1;
  return @foo @bar(v + a)
}

EDIT:

decorator @decorator {
  @log("A")
  @log("a")
  @log(1)
}

Do you think that when using this block-like syntax it would be more intuitive to see 1 a A or A a 1?

ljharb commented 5 years ago

You'd want to also be able to make a decorated class, and pass that as an argument.

nicolo-ribaudo commented 5 years ago

When using the decorator declaration syntax? Could you give an example?

jsg2021 commented 5 years ago

Do you think that when using this block-like syntax it would be more intuitive to see 1 a A or A a 1?

If its a block or block-like, I'd be inclined to expect A a 1.

ljharb commented 5 years ago

@nicolo-ribaudo super contrived, but:

decorator @foo(x) {
  let C;
  if (x) {
    C = @decorated classX {}
  } else {
    C =@decorated class {}
  }
  @wrapWith(C);
}

In general, if it takes arguments I expect it to behave like a function, which means it has a body of statements, and has a terminal value (which might be a list of decorators)

littledan commented 5 years ago

@ljharb I don't think this level of dynamic behavior would be consistent with design goals. At the same time, I'm not sure what syntax wouldn't suggest that things are dynamic, since so much in JS is dynamic. At some point, people will have to learn that decorators are more restricted. (In general, I'm not a big fan of this style of argument by "here's an example, and I expect it to work"--sometimes, it can lead to insights, but it's always possible to formulate impossible to achieve expectations.)

littledan commented 5 years ago

@nicolo-ribaudo I was wondering about this as well. I want something like that to be able to work, and it seems useful, but I'm not sure how to handle parts of it being part of what's returned and parts of it not.

ljharb commented 5 years ago

Another possibility could be this:

decorator @xyz(a, b) {
  statement;
  list;
  const f = @decorated class {};
  with;
  normal;
  block;
  semantics;
  @one @plus @decorators @must @be @the @last @items @in @the @block(f)?
}

iow, it could allow the full flexibility of any block, but still could offer the static guarantees about which decorators are composed to create the decorator the block is declaring?

rdking commented 5 years ago

Here's a thought. What if decorators could only take a single function as a parameter. This is more or less what @wrap, @register, & @initialize are currently like. In each of these cases, the built-in decorator calls the function to perform some action. In #253, I've suggested also adding @insert and @remove to this list.

If the goal is to reduce the complexity of decorators, then block structuring may be dragging in too many expectations. What if we change our perspective of decorators and say that they are event hooks into the class evaluation process? This is why I suggested that decorators only ever take a single function parameter. However, that's not the complete thought.

We also need to break from the idea that a decorator is a function. Even if it's a custom decorator, it needs to be restricted to only passing along its parameter to the decorators that compose it. The trick to making all of this flexible and still being able to do functional manipulation is to use high-order functions as the parameters to the decorators. But the question is how we go about dispensing with the appearance of a function in our decorator definitions.

decorator @xyz = @decorators @must @be @the @only @items @in @the @statement(arg);

If you need a more complicated decorator taking external data into account:

decorator @wtf = @something(arg(1, 'alpha')) @somethingElse(arg(2, 'beta'));

function fn(param) {
   return function fnfn(n, fArg) {
      let values = [
         ((p1, p2) => { /* do something interesting */ }).bind(null, fArg),
         ((p1, p2, p3) => { /* do something interesting */ }).bind(null, fArg, param)
      ];
      return values[n];
   }
}

@wtf(fn("your argument here"))
class C {
   ...
}
rdking commented 5 years ago

Here's an example of how to use decorators as I've described. This is something I'm probably in the severe minority for wanting, but I know I'll use it.

decorator @onPrototype = @insert(arg(0)) @remove(arg(1));

function relocate(n) {
   let retvals = [
      function move(mDesc) { mDesc.placement = "prototype"; return mDesc; },
      function del(mDesc) { return { key: mDesc.key, placement: mDesc.placement; } }
   ];
   return retvals[n];
}

class C {
   @onPrototype(relocate)
   pubField1 = 42;
}

assert(C.prototype.hasOwnProperty("pubField1"));
assert(C.prototype.pubField1 === 42);
pabloalmunia commented 5 years ago

I'm going to switch the draft explainer to use @trotyl's proposed syntax, but I'd still like to hear your suggestions about other alternatives, or thoughts on the tradeoffs.

Today we could spend some time to play by rewriting some of our decorators (developed with the Nov'18 specification) to the new proposal. There are our fist impresions:

This syntax is a bit confusing:

const @decorator = @(baz) => @foo @bar(baz);

for us this is simplest:

decorator @decorator(baz) {
  @foo @bar(baz)
}

This syntax is very similar (remember) to the body of the function and, therefore, we hope to also do the usual things that are included in the body of the function. It is not very difficult to understand that we are doing something different and therefore that it has its own rules and limitations.

We prefer that you do not use return and differentiate it from functions.

decorator @decorator(v) {
  let a = 1;
  @foo @bar(v + a)
}

Creating variables (as the previous example) with a lexical scope is a very good idea, but as a result we are opening a great door and any instruction of a function could make sense here. Which is the limit?

Well, it's only the first impression of an enthusiastic decorator builders team.

mbrowne commented 5 years ago

I think @rdking raised a good question about @ being part of the name of the decorator. What then is the precise meaning of @? I previously thought of @ as meaning "apply a decorator". If including @ makes it easier to learn and read the code, then I would say that's the better option, but I'm not sure that's the case.

I'm sure people can learn and get used to whatever syntax, but as with # being part of the name for private fields, we're treading in new territory here that's different from the way special tokens (like # and @) are usually defined. In other languages and other parts of JS itself (at least, off the top of my head), such special tokens are usually used either at the declaration site or at the usage site, but not both. @littledan mentioned that people seem to find it intuitive to include the @, which certainly is a point in its favor, but I'm hesitant to fully endorse it just yet.

mbrowne commented 5 years ago

I definitely like the idea of a decorator keyword though :)

littledan commented 5 years ago

@mbrowne Aside from intuition, making @ part of the name is sort of key to this proposal being statically analyzable, as explained in the FAQ. Do you have thoughts on how this could be done otherwise?

mbrowne commented 5 years ago

@littledan Ah. I was specifically thinking about the @ right after the decorator keyword, as in decorator @myDecorator—not literally eliminating all @ instances from your declaration example. Seems like it could just be decorator myDecorator. It's easier to explain with an example...just like @rdking suggested, the syntax could be:

decorator myDecorator(baz) {
  @foo @bar(baz)
}

Given this syntax, I think you can make the case here that even though @ doesn't appear before myDecorator, @ is appropriate for foo and bar because those decorators are actually being "used" there (as opposed to being declared, which is a totally different thing).

It seems to me that thanks to the decorator keyword, this would still be statically analyzable, but maybe I'm missing something.

littledan commented 5 years ago

Oh, I see what you mean. You are OK with the @ in import statements? This comes back to a matter of taste, I suppose. For me, it's easier if the declaration and usage look the same.

mbrowne commented 5 years ago

I hadn't thought about import statements; would have to reflect on that. Actually I'm undecided generally, but concerned about overloading @ too much with the decorator @myDecorator syntax.

rdking commented 5 years ago

@littledan I'm curious to know what you thought of the notion of using the built-in decorators as event handler registrars with the literal capability of re-writing or modifying the class' lexical scope.

robbiespeed commented 5 years ago

I prefer the decorator @foo syntax, it more clearly shows a difference from defining a function.

Including @ as part of the name I feel also provides consistency especially during import.

export decorator @foo { @bar(1) }
import { @foo } from './foo';

What's unclear to me is how this would work with default exports? Since on import a default export can be named whatever you like.

export default decorator @foo { @bar(1) }

Would it throw an error here:

import foo from './foo';

Similarly with named imports would this be an error:

import { @foo as foo } from 'foo';

I'm more worried about the default case, named param case seems easier to detect.

Presumably this would be totally valid though:

import { @foo as @bar1 } from 'foo';

In regards to allowing other statements inside the decorator definition, my gut tells me this would lead to issues in static analysis.

robbiespeed commented 5 years ago

How about this for the syntax:

decorator @foo(x, y): @a(x) @b @c(y);

This avoids any confusion that decorator declaration allows function behaviour like adding statements inside the declaration.

At first I thought maybe it would clash with typescripts use of : after function arguments to declare return value's type. It actually fits quite well though since the return value is just a composition of other decorators, which would always equate to the type of the decorator.

littledan commented 5 years ago

@rdking I don't know what you mean by event handler registrar.

@robbiespeed About my thoughts on those different import scenarios:

One reason I like @trotyl's proposed decorator syntax with a block is that it avoids the need for a mandatory semicolon at the end. We know some programmers don't like that syntax.

ljharb commented 5 years ago

There's also plenty of programmers that dislike a semicolon being optional; I don't think we should make any decisions about syntax because of semicolons in either direction.

littledan commented 5 years ago

Well, I agree generally, but using ,{} helps avoid the question. Also I find it created a stronger grouping and make it more obvious we are talking about a declaration, since it looks like a function declaration. This is all subjective, though.

robbiespeed commented 5 years ago

@littledan I see where you are coming from, semicolons are a highly contested topic. As far as I can tell though there's nothing stopping ASI from working on the syntax I proposed. If anything other than a decorator is written then it could act as a semicolon. The one gotcha with that is if you declare a decorator without semicolon right before the intended use of a decorator.

decorator @a: @b

@c
class Foo {

The @a decorator would end up composing @b @c, and Foo would remain undecorated.

In regards to imports and exports I think we need to find a way to allow default exports. It's quite unintuitive otherwise, especially for library authors.

Would it not be possible for the interpreter to detect that a default import is for a decorator and error if @ is missing?

I should clarify that in my earlier comment I was mostly worried about the user being able to detect when they are importing the decorator wrong. I don't think it's a valid reason to disallow default exports, just something to keep in mind.

littledan commented 5 years ago

Right, that's exactly the ASI issue I was referring to. Except, if we want to permit more things to be decorated in the future, the scope of the hazard grows. We have to reserve this syntactic space in advance. So basically a semicolon would be mandatory here. But there are other reasons why it's nice to have the decorator body grouped (e.g., more extension possibilities).

I guess a default-exported decorator could have the name @default, and default importing a decorator could use that name, right? We might want to prohibit certain cases, but that's about the extent of issues.

Lodin commented 5 years ago

Do I understand correctly that {} block won't allow declaring scoped variables inside? Why? It would be so convenient. What if I need to share a variable between @wrap and @register callbacks? I used to use this approach with the old proposal and actually don't see any reason to remove it.

littledan commented 5 years ago

Maybe we should allow this. I was thinking it could be a post-MVP feature, but maybe it doesn't have to be.

rdking commented 5 years ago

@littledan

@rdking I don't know what you mean by event handler registrar.

Consider a general process of evaluating a class in source code:

  1. Parse the definition into an AST. No event.
  2. For each class member: a. Create a class member descriptor from the AST for the member b. Fire a member event c. Use the return value from the event to replace the AST for the member
  3. Fire a class event a. Use the return value for the event to replace the AST for elements of the class definition
  4. Process resulting AST as current for a class.

The way to "register" for these events would be to use decorators. I've put more info on this concept of decorators here.

Morgul commented 5 years ago

Oh, I see what you mean. You are OK with the @ in import statements? This comes back to a matter of taste, I suppose. For me, it's easier if the declaration and usage look the same.

@littledan We've got a two edged sword here with @ being part of the name. It implies things that I don't think are true, and I personally would find confusing:

Given this declaration:

decorator @foobar {
  @foo @bar
}

Then the following import syntax/usage makes sense:

import { @foobar } from './myDecoratorLib';

@foobar
function doStuff() {
    // ...
}

However, it implies that the following would work:

decorator foobar {
  @foo @bar
}
import { foobar } from './myDecoratorLib';

foobar
function doStuff() {
    // ...
}

Or, possibly worse:

decorator @foobar {
  @foo @bar
}
import { @foobar as foobar } from './myDecoratorLib';

foobar
function doStuff() {
    // ...
}

Part of the way I think about it is this: for fields # denotes private, but it's still a field either way. In this case, @ has much different behavior, it's required to be a decorator at all, in the declaration, import or usage. That's brand new territory for JS, and I'm not sure I like it. (I hate # for private as well, though, full disclaimer.)

IMHO, @ should be required to use a decorator, and only that. Import and declaration should not involve @, unless we're replacing the decorator keyword with at, and I don't like that as much.

Put another way, show the following to 20 JS devs of all skill levels, and I'll be mildly shocked if more than one is confused by what's going on:

// myDecoratorLib.js
export decorator foobar {
  @foo @bar
}
import { foobar } from './myDecoratorLib';

@foobar
function doStuff() {
    // ...
}
Lodin commented 5 years ago

@Morgul, I'm not sure I can find it but this question is already raised somewhere in this repo and there is an answer for it.

1) It doesn't imply that you can declare decorator without @, this kind of action is simply forbidden and treated as a syntax error.

2) Decorator renaming goes the same way: since decorator is not a usual JavaScript element, you cannot assign a name without @ to it. It's a syntax error again.

It was made to allow affected classes optimization by permitting decorators run during the earliest stages of initialization. If you remove the @ symbol from the decorator import or declaration, engine won't be able to recognize the decorator as a decorator and won't be able to apply optimizations.

rdking commented 5 years ago

@Lodin

I'm not sure I can find it but this question is already raised somewhere in this repo and there is an answer for it.

I think I was among the ones to raise this issue before. Here's a flat comparison between class-fields and this decorators proposal's syntax;

class-fieldsdecorators
Declaration Accepted#fielddecorator @dec
Declaration Rejectedprivate #field
private field
decorator dec

The private #field notation was rejected in part because having both private and # was redundant and "not as ergonomic". However, the same pattern that was rejected for private fields is being pushed for decorators. That's inconsistent.

Since using @dec {} would preclude the possibility of decorators on objects, and doesn't really resemble a top-level declaration in ES, the decorator token is being offered as a keyword. This is good. However, offering a pattern that was rejected for one kind of declaration as the new pattern for a different declaration doesn't make sense. The same reasoning applies in both cases.

Since decorator aides readability and unblocks future options, but <declarator> <useToken><Identifier> is a rejected pattern, that leaves decorator dec {} as the most logical and viable candidate for the syntax.

mbrowne commented 5 years ago

@rdking Good point; now that you mention it, I find that inconsistency bothersome as well. But I'm not sure the inconsistency will actually matter to the average developer. Also, developers will probably be writing a lot more private fields declarations (often many per class) than custom decorators, so I think there's at least a small case to be made that the difference is justified. So in the end, if developers find the decorator @ syntax easier to understand and more usable, I'm for it. But last I heard, there hadn't really been enough feedback yet to establish that.

rdking commented 5 years ago

@mbrowne Barring any subjective distaste for the syntax, #field is acceptable for a field declarations because it is in keeping with the existing, default keyword-less declaration style of existing class members (though a reasonable argument could be made for using a keyword since private is a modifier in the same way as async, static, etc...).

On the other hand, decorators are akin to functions in terms of syntax. In this case, the keyword-less argument doesn't apply. IMO, that's the reason for there being a difference at all. However, I don't see that as reason enough to keep a redundancy that was decried in other proposals.

glen-84 commented 5 years ago

Most of the people I've talked to are really positive about @ being part of the name.

I don't think I like it, but that's probably not too important.

With npm scopes you'd end up with a lot of symbols again ...

import { @foo as @bar } from "@baz/example";

I'm curious about this new direction. Were there major issues with the current decorators? Did developers complain about them being functions, or not being named with a symbol prefix?

You have class X, function X, why not decorator X? Wouldn't it be more consistent for them to be values? Are there no use cases where you might pass a decorator into a function and maybe decorate a child function or something? IDK, just wondering.

For me, the @ is just syntax for "attaching" the decorator, not part of its name.

Java annotations: @ not part of the name. PHP (pseudo) annotations: @ not part of the name. Dart annotations: @ not part of the name. C# attributes: [, ] not part of the name. etc.

Current decorators, from the outside at least, appear to work well and look good. I just hope that this proposal changes things for good technical reasons.

Morgul commented 5 years ago

Current decorators, from the outside at least, appear to work well and look good. I just hope that this proposal changes things for good technical reasons.

@glen-84 I think you've summed up my feelings on the JS decorator proposal rather well with those two sentences.

I'm increasingly fearful that this proposal's pretty far off the reservation and is going to be very frustrating to use. The @ being part of the name is just a symptom that this iteration of the spec is far removed from what I think "in the trenches" developers would expect.

littledan commented 5 years ago

@glen-84 @Morgul The FAQ tries to document the issues with the previous decorators proposals, which this version attempts to solve. Fundamentally, making @ part of the name and making decorators not be first-class JavaScript values gives the possibility of making decorators more statically analyzable. I hope the simple "composition" logic will also make it easy to define decorators.

rdking commented 5 years ago

@littledan Can you explain why "static analysis" is so critically important for an action that takes place 1 time per class declaration? This confuses me just a bit. Even if you had 100 classes, each using the exact same combination of decorators, the static analysis for each would render a different result per class. Meanwhile, a dynamic analysis would do precisely the same thing for each class because the analysis would only be performed once. What did I miss?

littledan commented 5 years ago

@rdking Browsers have found that startup time is heavily affected by how long it takes to get things to run the first time. I think it'd be a shame if use of decorators led to slow page load time--this could lead to advice to avoid them in certain cases.

rdking commented 5 years ago

Ok. Good argument. Here's a counter: No tool with many uses is solely the most effective tool in 100% of its use cases. Further, there are ways (design patterns) to get around the slow page load issue that would even work if decorators posed such a problem.

So my question is this: Is ensuring that decorators always provide the shortest possible initial processing time truly worth removing so much useful, well understood functionality? From a different perspective, I could ask this: If decorators cannot mutate the shape of the class, which seems to be one of the main uses from the older proposal, can it reasonably be said that correcting for the loss of load-time speed, which can be worked around by the developer, is worth the complete loss of a critical feature which cannot be worked around?

robbiespeed commented 5 years ago

@littledan there's plenty of transpiled classic decorator use in the wild today, it would be nice to see how much those have affected startup speeds by profiling time spent applying decorators. They would have a higher impact than any native implementation. If it turns out that the affect is still minimal in the majority of use cases, then it may be worth rethinking the benefits of this new syntax.

I feel like this syntax's strength isn't so much that it's static, but that it's a much simpler interface compared to old stage 2 decorators. Another option that keeps that simplicity is #277 which has the added benefits of polyfilling, and not introducing a valueless entity with it's own unique definition syntax.

littledan commented 5 years ago

@rdking

is worth the complete loss of a critical feature which cannot be worked around?

I don't think we're facing the potential loss of a feature. I agree that decorators are a critical feature, and the goal of this proposal is to make them happen.

@robbiespeed The old stage 2 decorators were benchmarked and found to cause significant overhead. I am not aware of an implementation of the idea in #277, but I suspect it would also have appreciable overhead (though probably not as much as the previous version). In general, I believe #277 would not be as implementation-friendly, and not permit as much static analysis. I'm trying to encourage more discussion among JIT implementers and the proponents of that proposal to investigate this issue further.

rdking commented 5 years ago

I don't think we're facing the potential loss of a feature. I agree that decorators are a critical feature, and the goal of this proposal is to make them happen.

Can I take this as you meaning that you have no intention of letting decorators see an initial release without the ability to mutate (add/remove/modify) class members?

littledan commented 5 years ago

@rdking This decorator proposal includes @register, which can be used for this sort of functionality in practice, so I believe that this feature requirement is met.

rdking commented 5 years ago

@littledan $.50 short. @register allows for manipulation of the prototype and public fields, but private fields are still essentially untouchable. Further, you cannot create new fields using @register. Where fields are concerned, add/remove functionality is missing, and modify is missing for private fields. So no, this feature is not met.

littledan commented 5 years ago

@rdking It's true that this proposal is less capable when it comes to private fields and methods. I'm comfortable with these restrictions for the initial release. The feature set is based on an analysis of reported use cases.