tc39 / proposal-decorators

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

@deco before or after export keyword? #69

Closed hax closed 1 year ago

hax commented 6 years ago
@deco
export class A {}

or

export @deco class A {}

or allow both?

rbuckton commented 6 years ago

My preference (and the current TypeScript implementation) is before export. More complex decorators like Angular's Component decorator often span multiple lines and the export keyword would end up oddly orphaned at the top otherwise.

export
@dec1
@dec2({
  option1: "foo" 
})
class C {
} 

// vs

@dec1
@dec2({
  option1: "foo" 
})
export class C {
} 
rbuckton commented 6 years ago

I suppose "both" might be an acceptable mix though:

@dec1
@dec2({
  option1: "foo" 
})
export @sealed class C {
} 
littledan commented 6 years ago

The current proposal only permits decorators after export. The DecoratorList is part of the ClassDeclaration, which is the thing that the export keyword. Should we revisit this decision?

This might be a change vs previous versions, which should be documented as part of https://github.com/tc39/proposal-decorators/issues/50 .

rbuckton commented 6 years ago

I recall discussions with @wycats over the past two years where we discussed this and I had thought we had settled on "before export" in the past.

One of the other reasons for TypeScript's implementation is that we have other possible keywords that can come before class (i.e. declare and abstract) and it seemed weird to write:

export
@decorator1
@decorator2
abstract class C {
}

With multi-line decorators you can end up losing track of the export keyword when looking at the class. It also fairly common to want to refactor your code by adding the export keyword and you might naturally assume you just put the keyword before class (which is the case in other languages like Java (annotations) and C# (attributes)).

rbuckton commented 6 years ago

The only other part of the conversation I recall was whether we wanted to eventually allow users to decorate an export binding separate from the class itself. Whether we would ever do such a thing and what that would mean is unclear.

I had suggested an extension to the syntax to support this in a way similar to how C# attributes allow you to specify placement, via a prefix:

[assembly: AssemblyVersion("1.0.0")]

class C {
  [return: MarshalAs(UnmanagedType.LPWStr)]
  string Method() { ... }
}

We could do the same thing with decorators:


@decorator1
@decorator2
@export: decorator3 // decorates the export binding
export class C {
  @route("/foo")
  @method: permit("administrators") // decorates the method (optional here)
  @return: serializeAs("json") // decorates the return value
  method() { ... }
}

I'm not necessarily saying we have to figure out what those mean, but that there's room in the design space to put together something meaningful that isn't dependent on whether the decorator is before or after the export keyword.

doodadjs commented 6 years ago

export is static, so I think decorating it is impossible. I think that's better to have the class's decorators just before the class keyword because it clearly shows that the decorators apply to the class, and not to export.

ljharb commented 6 years ago

Given that export isn't the declaration nor the thing being decorated, I think "after export" is the only thing that makes sense.

In other words, you don't need to "keep track" of export when you're looking at a long chain of decorators, because the fact that it's exported isn't directly relevant to what's being decorated and how.

rbuckton commented 6 years ago

What about if a future proposal adds other keywords before class, for example abstract, final, sealed, static, etc.? Those are logically associated with the class and not the ExportDeclaration, so if they are added there will be an odd mix of export before and these after.

When TypeScript adds support for ES decorators it will be an all-or-nothing switch, so we can definitely support export-before. Since TypeScript has export-after currently, I will see if I can get some feedback from our community as to whether they find our current syntax ergonomic or unergonomic. Having used it myself for a few years now, I personally find export-after the most ergonomic, easiest to refactor, and visually appealing.

doodadjs commented 6 years ago

That could be :

export @mydecorator abstract class {}

But I think abstract, final, sealed, static and etc. will probably be decorators, so :

export @mydecorator @abstract class {}
littledan commented 6 years ago

What about if a future proposal adds other keywords before class, for example abstract, final, sealed, static, etc.? Those are logically associated with the class and not the ExportDeclaration, so if they are added there will be an odd mix of export before and these after.

Why would these keywords make sense before export rather than after? Would @ljharb 's logic in https://github.com/tc39/proposal-decorators/issues/69#issuecomment-370246996 apply differently to decorators vs these keywords?

rbuckton commented 6 years ago

I'm not talking about those keywords before export, but rather the inconsistency of, say:

export
@dec
abstract class C {}

// vs

@dec
export abstract class C {} 

In discussing this with others on the TypeScript team, so far the consensus is that we haven't yet heard any negative feedback about our current placement of "before export", which generally indicates that our customers have been happy or at least accepting of that behavior after having decorators as a feature for several years.

Also, there are some things that can't currently be done in a decorator (i.e. final or sealed) so we can't necessarily wash our hands of the possibility of other future keywords.

ljharb commented 6 years ago

I guess i don’t understand the inconsistency. If you add or delete the export keyword, the declaration should be the same, conceptually - id find it very inconsistent if the ordering wasn’t “export”, “decorators for the thing”, “thing I’m exporting”.

rbuckton commented 6 years ago

Coming from C#, I find the separation of the export keyword from the class keyword by its decorators confusing. In C#, you would write this:

[attr1]
[attr2]
public abstract class C {
}

Where public is what makes the class reachable outside of the assembly (i.e. exported). If export needs to be before decorators, it seems like a refactoring pitfall:

class B {
}

@dec1
@dec2
class C { 
}

If I want to export B, I add export before the class keyword. If I want to export C its easy to assume that export goes before the class keyword, just like async goes before the function keyword. It all depends on how you view export. The spec treats it as a declaration in its own right, but its fairly easy for someone to assume export is just a modifier for class just like static is a modifier for a method:

class C {
  @dec1
  @dec2
  method() {}
}

I would refactor method by adding static after the decorators, but before the method name.

rbuckton commented 6 years ago

I'd be happy if the spec allowed me to chose one or the other as a personal preference, something like (simplified for brevity's sake):

ExportDeclaration:
  DecoratorList `export` ClassDeclaration[~Decoratable]
  `export` Declaration[+Decoratable]

ClassDecoratorList[Decoratable]:
  [empty]
  [+Decoratable] DecoratorList

ClassDeclaration[Decoratable]:
  ClassDecoratorList[?Decoratable] `class` BindingIdentifier ...  

Something like that would allow you to choose either export @dec class C {} or @dec export class C {}, but would disallow @dec export @dec class C {}.

doodadjs commented 6 years ago

@rbuckton The question is : What do you want to decorate with @decorator (while reading from Left-To-Right) ?

// Decorate "export" ?
@decorator export abstract class A {}

// Decorate the abstract class ?
export @decorator abstract class A {}

// Decorate a method ?
export abstract class A {
    @decorator
    method() {}
}
littledan commented 6 years ago

OK, if TS has decorators before export, we are talking about a fairly large compatibility risk (where this proposal attempts to not require much changes in the decorator usage side, even if we are fine with requiring the definitions to change). For mitigations, we could

Does anyone know what syntax Babel uses here?

hax commented 6 years ago

As I understand Babel have to follow TS because it's the goal of Babel 7 to compile TS to JS.

rbuckton commented 6 years ago

As I send earlier, using ES decorators will be a binary choice. If we end up choosing export-before then that's what TS will use and the old grammer would only be used when - - experimentalDecorators is set. We won't allow mixing and matching as the semantics are so different. Since you have to refactor anyways its less of a compatibility risk.

DanielRosenwasser commented 6 years ago

@mhegazy made a good point offline that not once in the almost-3 years since we shipped our decorators can we recall a complaint about the syntactic order of decorators with respect to the export keyword. If possible we'd strongly prefer either the decorators-before-export grammar (our current implementation), or the option for either style.

Since you have to refactor anyways its less of a compatibility risk.

As I understand it, decorator implementations can provide functionality that works under the older-style semantics as well as the newer-style semantics, so in those cases, you really are just providing a syntactic break with no provided semantic value.

ljharb commented 6 years ago

It’s worth noting that the lack of a complaint with “before export” isn’t the same as the presence of one with “after export”, especially in the context of build-time verifications like TS affords.

Ultimately, I would prefer we only choose one, and not permit both, whatever the choice (I’ve expressed my preference above :-) )

hax commented 6 years ago

Before I raised this issue, I searched related proposal repos to find usage of export with decorators, here are the results:

  1. From https://github.com/tc39/proposal-decorators-previous/issues/35#issuecomment-323215132 by @zenparsing

    @connect(...)
    export class B {
    render() {
    // ...
    }
    }
  2. From https://github.com/tc39/proposal-decorators-previous/issues/41#issue-260027055

    
    @customElement('nav-bar')
    export class NavBar {}

@useShadowDOM @customElement('my-expander') export class Expander { @bindable isExpanded = false; @bindable header; }

NOTE: This code example is coming from the old decorator draft: https://tc39.github.io/proposal-decorators-previous/#example-aurelia-customElement

3. From https://github.com/tc39/proposal-decorators-previous/issues/2#issuecomment-225314165 by @justinfagnani 
```js
@define('my-element')
export class MyElement extends HTMLElement {}

All three use @deco export class form.

kt3k commented 6 years ago

I checked babel's implementation. It seems allowing only decorators-before-export.

$ cat .babelrc 
{
  "plugins": [
    "transform-decorators-legacy"
  ]
}
$ cat test0.js 
@deco export class A {
}
$ npx babel test0.js 
var _class;

export let A = deco(_class = class A {}) || _class;

$ cat test1.js 
export @deco class A {
}
$ npx babel test1.js 
test1.js: Unexpected token, expected { (1:7)
loganfsmyth commented 6 years ago

Babel's implementation of the current decorator spec has not landed yet. The existing plugin is called -legacy specifically because it doesn't reflect the current spec.

Babel 7.x's parser has a decorators2 plugin that implements the current proposal's placement of decorators after export, and prints a helpful message if found beforehand, e.g.

Using the export keyword between a decorator and a class is not allowed. Please use export @dec class instead

for

@dec
export default class Foo {}

for example.

The 7.x transform does not yet expose this parser plugin for use, but we intend to as we transition to aligning with the current proposal.

cztomsik commented 6 years ago

Decos should be before export because that's how it reads better. We read the code more than we write it. This should be changed before it's too late.

ljharb commented 6 years ago

I don’t agree; to me, before reads like it applies to the exporting; after reads like it applies to the thing being exported.

cztomsik commented 6 years ago

Correct me if I'm wrong but this is where it's going right now and I don't like it at all

export @Component({
  template: `
    <div>...</div>
  `
}) @AnotherMeta({
  x: ...
}) class Counter {
 ...
}

BTW: I think somebody will eventually write babel-plugin which will swap the order for you which means people will stick with @deco export ... and the spec will get updated anyway

jsg2021 commented 6 years ago

I originally thought it was weird to move the decorators between the export and class, but I’ve become used to it tho. I agree with both sides however.

jsg2021 commented 6 years ago

The pattern I've come to is

export default
@decorator0
@decorator1()
@decorator2({meh: true})
class Foo extends Bar {
...
}
justinfagnani commented 6 years ago

BTW: I think somebody will eventually write babel-plugin which will swap the order for you which means people will stick with @deco export ... and the spec will get updated anyway

This is a really unproductive thing to say.

hax commented 6 years ago

This is a really unproductive thing to say.

While it's may unproductive, I see it as helpless words.

To be fair, in the history of JavaScript and Web platform standardization, most decisions are made by a few people, or vendors. When they did some unproductive things (I don't want to give examples here), the programmers were almost helpless. So I hope we could use our sympathy, and be tolerant to the unproductive thing from the programmers.

ljharb commented 6 years ago

@hax the people making the decisions aren't saying "too bad, we're doing it anyways" - they simply might have reasoning that some programmers disagree with. That comment was purely unproductive, and feeling frustrated doesn't mean it's a good idea to go down the road of "i'm going to force my preference on you if you don't give me what i want" (not that it would be possible to do that with babel, nor that babel users' preferences could force an unwise spec change)

cztomsik commented 6 years ago

Sorry, I just wanted to point out, that babel is here and doing it is super easy, it's like 30 minutes of hacking, npm publish and voila, you can use deco before export even if it's not in spec.

And if there is enough people doing angular and writing templates (a lot of lines) in decorators (which I believe there is) somebody will eventually do it.

You can have many decorators (with complex params) and in my opinion, decorators look better when they're stacked. And because export can't be on its own line (bc of parsing), the only nice way is to move the whole stack of decorators before export class

jsg2021 commented 6 years ago

Or export in a separate statement.

@decorator0
@decorator1()
@decorator2({meh: true})
class Foo extends Bar {
...
}

export { Foo };
//or
export default Foo;

Sent with GitHawk

littledan commented 6 years ago

@cztomsik I think it's beneficial to have alignment between Babel and TC39 long-term. Fragmenting into lots of dialects on these syntax questions doesn't really help anyone. @loganfsmyth Thanks for putting in the effort to generate such a good error message in this case.

cztomsik commented 6 years ago

OFC it is. Anyway, I'd really love to hear a reasoning behind this decision because I was discussing this with a lot of people doing js everyday and every single one of them would prefer deco before export (because its less typing and duplication then exporting the class later)

@deco
@deco({...})
@deco
class Clz
export Clz
littledan commented 6 years ago

@cztomsik I don't quite understand--how is the decorator first less typing or duplication? It seems like the same amount of typing, just in a different order. Note that this form is supported (I believe):

export
@deco
class Clz

Anyway, given the activity on this issue, I'd like to review it with the other decorators co-champions before the May meeting for reconsideration.

cztomsik commented 6 years ago

Oh, I thought export with following newline would cause SyntaxError and obviously I was wrong, sorry for that.

hax commented 6 years ago

@littledan

how is the decorator first less typing or duplication?

I guess it's not about typing but the reading/understanding of the code. export is about external interface, but decorators are implementation details. To understand what is exported, you just need to read "export TYPE NAME" (where TYPE is function, class, const, etc.) which give you all the information.

Normally we read the code in the order of interface, then details. With the decorators, it becomes

export // external api, but don't know what is exported now
@deco1 // declarative, but still implementation details
@deco2
@deco3
class Test { // local binding, but also external api if there is export keyword lines before
  // details
}

or

@deco1 // declarative implementation details
@deco2
@deco3
export class Test { // external api, and Test is also the name for the local binding
  // details
}

Inject implementation details between export and TYPE NAME seems switching more context when reading the code.

ljharb commented 6 years ago

The implementation details are what’s exported.

hax commented 6 years ago

@ljharb

The implementation details are what’s exported.

I don't think so. The code

import {Test} from './test.js'
const test = new Test()

can be parsed before the test.js loading and execution, which means only names are exported. The implementation details are linked later.

More important, I'm talking about the mental model of the programmers. To be honest, decorators are more declarative which may convey the higher-level abstraction than normal implementation details like class body, but I assume most programmers will still see decorators are much more internal than class name and method signatures, because we need name and type info but should not rely on the implementation details when writing the codes.

Note, I myself do not have a very strong opinion on this issue, just try to use my empathy for those who prefer decorators before export.

cztomsik commented 6 years ago

@hax Agree (but I understand this is highly subjective)

hax commented 6 years ago

When I wrote last comment, I just realize that this issue also affect other class elements. We do not have many modifier keywords now (only static uptonow), but if we have, for example hidden in class 1.1 proposal, TS-like readonly as extension for field proposals, this will be much general issue:

export
@deco1 @deco2
class A {
  readonly
  @deco1 @deco2
  prop
  hidden
  @deco1 @deco2
  method() {...}
  static
  @deco1 @deco2
  method() {...}
}

or

@deco1 @deco2
export class A {
  @deco1 @deco2
  readonly prop
  @deco1 @deco2
  hidden method() {}
  @deco1 @deco2
  static method() {}
}

or have different preferences in each cases? (I believe current proposal is close to the latter but make export as the only exception.)

k1r0s commented 6 years ago

I would prefer decorators to come before exports. And I subscribe the previous arguments from @hax

Thinking about methods and properties with modifiers like static or whatever..

Decorators should be on top of everything so you're able to write it in a way that you can remove the whole line without dealing with language principles

littledan commented 6 years ago

At this point, I am leaning towards the decorator coming before the export keyword, following @rbuckton's and @k1r0s's arguments.

We discussed this issue in the decorators champions call, and some other people seemed convinced as well. We will revisit this to make a more solid recommendation to TC39. @rbuckton signed up to write spec text for this syntax.

littledan commented 6 years ago

@rbuckton @nicolo-ribaudo and I discussed this issue. For the reasons described above, we're all in favor of the change. We need to follow up with @wycats on the possibility of future export decorators, but at the same time, @rbuckton has described a good path for the combination above (if we ever figure out what export decorators might do). Ron volunteered to write a patch for the syntax change.

littledan commented 6 years ago

@wycats @bterlson @rbuckton and I discussed this issue again. At this point, we're all convinced that we should make this change. @rbuckton signed up for writing the patch.

doodadjs commented 6 years ago

@jsg2021 Sorry for being off topic in 92 following your comment...

How will you decorate "export" when needed if decorating a class before export is retained ?

littledan commented 6 years ago

@doodadjs See https://github.com/tc39/proposal-decorators/issues/69#issuecomment-370239981 for a syntax by @rbuckton .

doodadjs commented 6 years ago

@littledan Seen that, and I disagree.

doodadjs commented 6 years ago

To be more precise, if I want to decorate my house, I'll not decorate my neighborhood's house, with a link to my own house...