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

@deco before or after export keyword? #69

Closed
hax opened this issue Mar 4, 2018 · 377 comments
Closed

@deco before or after export keyword? #69

hax opened this issue Mar 4, 2018 · 377 comments
Assignees

Comments

@hax
Copy link
Member

hax commented Mar 4, 2018

@deco
export class A {}

or

export @deco class A {}

or allow both?

@rbuckton
Copy link
Collaborator

rbuckton commented Mar 4, 2018

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
Copy link
Collaborator

rbuckton commented Mar 4, 2018

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

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

@littledan
Copy link
Member

littledan commented Mar 4, 2018

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 #50 .

@rbuckton
Copy link
Collaborator

rbuckton commented Mar 4, 2018

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
Copy link
Collaborator

rbuckton commented Mar 4, 2018

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
Copy link

doodadjs commented Mar 4, 2018

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
Copy link
Member

ljharb commented Mar 4, 2018

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
Copy link
Collaborator

rbuckton commented Mar 4, 2018

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
Copy link

doodadjs commented Mar 4, 2018

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
Copy link
Member

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 #69 (comment) apply differently to decorators vs these keywords?

@rbuckton
Copy link
Collaborator

rbuckton commented Mar 4, 2018

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
Copy link
Member

ljharb commented Mar 4, 2018

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
Copy link
Collaborator

rbuckton commented Mar 5, 2018

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
Copy link
Collaborator

rbuckton commented Mar 5, 2018

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
Copy link

doodadjs commented Mar 5, 2018

@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
Copy link
Member

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

  • Allow either placement, with identical semantics, as @rbuckton is suggesting
  • Permit only the before-export placement
  • In the JS spec, allow decorators only after export, but then TS has an extension to allow them before if it feels that this is necessary

Does anyone know what syntax Babel uses here?

@hax
Copy link
Member Author

hax commented Mar 5, 2018

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

@rbuckton
Copy link
Collaborator

rbuckton commented Mar 5, 2018

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
Copy link
Member

@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
Copy link
Member

ljharb commented Mar 5, 2018

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
Copy link
Member Author

hax commented Mar 5, 2018

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

  1. From Decorate a function proposal-decorators-previous#35 (comment) by @zenparsing
@connect(...)
export class B {
  render() {
    // ...
  }
}
  1. From Explicit call-out about syntactic diffs from legacy implementations? (unmodified ExportDeclaration) proposal-decorators-previous#41 (comment)
@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

  1. From Use case: class/function registration proposal-decorators-previous#2 (comment) by @justinfagnani
@define('my-element')
export class MyElement extends HTMLElement {}

All three use @deco export class form.

@kt3k
Copy link
Contributor

kt3k commented Mar 5, 2018

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
Copy link

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
Copy link

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
Copy link
Member

ljharb commented Mar 23, 2018

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
Copy link

cztomsik commented Mar 23, 2018

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
Copy link

jsg2021 commented Mar 23, 2018

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
Copy link

jsg2021 commented Mar 23, 2018

The pattern I've come to is

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

@justinfagnani
Copy link

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
Copy link
Member Author

hax commented Mar 23, 2018

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.

@hax
Copy link
Member Author

hax commented Feb 2, 2023

@ljharb Yes, u can replace the entire value to a very different thing, but IMO it's not a common practice and I think it's a bad thing in practice. We allow that just because in JS we have no way to disallow that.

@ljharb
Copy link
Member

ljharb commented Feb 2, 2023

Of course we have a way to disallow it - decorators explicitly supports that use case because TC39 believes it is NOT a bad practice. If we thought it was a bad practice it wouldn't be possible.

@hax
Copy link
Member Author

hax commented Feb 2, 2023

I don't understand how JS spec can disallow a decorator change a class into a function?

@ljharb
Copy link
Member

ljharb commented Feb 2, 2023

A class is already a function - decorators explicitly allow the capability to replace the value being decorated (when it's a class decorator). It would be trivial to ban that, but that was a critical use case for the decorator proposal - a good and necessary practice, not a bad one.

@keithamus
Copy link
Member

This is quite a long thread so I don't know if it's been discussed already, but I took a look at existing precedent which may help inform which way the JS community leans. My search seems to indicate that many codebases favour @_ export class, with limited number that favour export @_ class.

Since #69 (comment) Babel has a boolean to toggle this. A simple search of decoratorsBeforeExport: false returns 410 files. decoratorsBeforeExport: true returns 1.3k files. It's worth noting that decoratorsBeforeExport defaults to false.

Searching for /^export @(.+)/ (language:JavaScript OR language:TypeScript) returns 520 files. Searching /^@[^\n]+\n?export/ (language:JavaScript OR language:TypeScript) returns 987k files. I'm sure someone could write a more robust query to provide finer grained results but it seems from a cursory glance decorators before export are several orders of magnitude higher in occurrence.

GitHub's own codebase has uses @_ export class exclusively, we have no instances of export @_ class.

@hax
Copy link
Member Author

hax commented Feb 2, 2023

A class is already a function - decorators explicitly allow the capability to replace the value being decorated (when it's a class decorator). It would be trivial to ban that, but that was a critical use case for the decorator proposal - a good and necessary practice, not a bad one.

Of coz there are many good cases. My point is not there is no good cases, but there are bad cases (for example replace a class to arrow function or async generators or any function which have a very different shape of the original class), and we have no way to disallow them in language layer, it must be developers to take the responsibility.

So what I am arguing is, even decorators have the power to replace the class to very different things (and in such cases, the decorators may be more important than TYPE), it's not the common cases. My analysis of readability is based on the common cases that the importance of the information for reading code normally is:

binding name = export > TYPE > decorators > implementation details.

This is why @deco1 @deco2 export TYPE NAME is better than export @deco1 @deco2 TYPE NAME in most cases, because the latter make the most important things (NAME and export) in a much long distance.

@pabloalmunia
Copy link
Contributor

pabloalmunia commented Feb 2, 2023

I add again a vision already expressed (most of them have already been exposed).

If I am not mistaken, the order of execution of the decorators is from right to left, bottom to top, i.e. the decorator closest to the class is the first to receive and decorate it, then the next, and so on.

@decorator4th
@decorator3th
@decorator2th
@decorator1th
class A {
}

Finally, the result of all this decorating process is exported.

export 
@decorator4th
@decorator3th
@decorator2th
@decorator1th
class A {
}

In my humble opinion, this is common sense execution of the different parts.

If you write export before decorators, it denotes that the class is exported before and decorators are executed after the export action, as a result, you can say that the exported class is not decorated.

@decorator4th
@decorator3th
@decorator2th
@decorator1th
export 
class A {
}

Of course, we can understand lexical problems, performance problems, but please consider the psychological behavior.

@rbuckton
Copy link
Collaborator

rbuckton commented Feb 2, 2023

I add again a vision already expressed (most of them have already been exposed).

If I am not mistaken, the order of execution of the decorators is from right to left, bottom to top, i.e. the decorator closest to the class is the first to receive and decorate it, then the next, and so on.

@decorator4th
@decorator3th
@decorator2th
@decorator1th
class A {
}

Finally, the result of all this decorating process is exported.

export 
@decorator4th
@decorator3th
@decorator2th
@decorator1th
class A {
}

In my humble opinion, this is common sense execution of the different parts.

If you write export before decorators, it denotes that the class is exported before and decorators are executed after the export action, as a result, you can say that the exported class is not decorated.

@decorator4th
@decorator3th
@decorator2th
@decorator1th
export 
class A {
}

Of course, we can understand lexical problems, performance problems, but please consider the psychological behavior.

export is not an imperative action. It is declarative and occurs during module initialization, not during module body evaluation. All of the following are semantically equivalent:

export class C {}

class C {}
export { C };

export { C };
class C {}

The only imperative behavior associated with export is initializing the binding that is exported.

@david0178418
Copy link

@pzuraq

I did not say that I thought it was unused, just that in my experience it is relatively uncommon

My question was dismissed by suggesting any usage of it was "suboptimal/unusual". This is clearly not the case as I explained a usage and pointed to a real world example.

It feels like this line of thinking is overrotating on the idea that because one form of export is separable from the definition and clearly not a modifier, we must treat all of them like that.

This is the crux of the matter. Why conceptually overload the export keyword and introduce an additional mental model when one works?

So, to return to my original question: From your experience, how do the devs who feel this is breaking up the class definition into multiple lines react to export { Foo, Bar as Baz };?

I'd imagine this looks really weird, considering the majority of what they've seen is based on the early implementations by babel and typescript that took a swing at things before it was fully thought out. Make no mistake, this was important work, but it seems weird to keep the unnecessary complexity after it's been identified just because of momentum. JS already has plenty of baggage to carry that can't be changed, so it seems unwise to introduce more when there's a chance to keep things simple and consistent from the get.

From here, I'll switch back to being a lurker. I'm quite disappointed to have seen this issue resurrected from the dead, but I'm also excited about decorators finally being unstuck and being able to progress after years of being hung up on this.

@Kukkimonsuta
Copy link

People are often connecting "export" with "public" and "decorators" with "annotations/attributes" which would make ES inconsistent with majority of other languages having these features. While technically they are different they mostly serve the same purpose from the users pov so you can't really blame people for assuming they are the same.

I'd like to also point out we're talking about making what's essentially syntactic sugar potentially require more lines (in non-trivial cases where everything doesn't fit on one line), which is quite unfortunate.

When we turn

@Injectable({
  providedIn: 'root'
})
export class A {

}

into

export
    @Injectable({
        providedIn: 'root'
    })
    class A {

    }

Might as well go with:

@Injectable({
  providedIn: 'root'
})
class A {
    
}
export { A };

@pzuraq
Copy link
Collaborator

pzuraq commented Feb 2, 2023

@david0178418

My question was dismissed by suggesting any usage of it was "suboptimal/unusual". This is clearly not the case as I explained a usage and pointed to a real world example.

This has felt like my entire interaction in this thread. The way I (and many others) experience the export keyword is dismissed completely in favor of a different interpretation which feels more consistent to others. I have acknowledged multiple times that I believe this is an actually valid interpretation, but I disagree with it and it's not how my mental model works. I have not seen anyone in this thread on the decorators-after side do the same and admit that decorators-before has a point, and that it's a valid mental model. Instead, it's just repeatedly hammering home that because this mental model is "simpler", it is therefore better and everyone should use it.

I'd imagine this looks really weird, considering the majority of what they've seen is based on the early implementations by babel and typescript that took a swing at things before it was fully thought out. Make no mistake, this was important work, but it seems weird to keep the unnecessary complexity after it's been identified just because of momentum

It feels like you're completely ignoring what I'm trying to communicate to you here. Look, export is contextual to me - when used in directly in a class declaration, it reads as a modifier. When used separately, it reads like a somewhat imperative statement. As @rbuckton has pointed out, it isn't really, but that is beside the point.

I have zero issues with this mental model, and it works quite well for me. It also, in my experience, works quite well for a lot of people. We do not find export { Foo, Bar as Baz }; weird or bad or hard to understand, it's just contextually different, and it's not the first tool we reach for because it means typing more, which is usually something that devs like to avoid.

@pabloalmunia
Copy link
Contributor

pabloalmunia commented Feb 2, 2023

export is not an imperative action. It is declarative and occurs during module initialization, not during module body evaluation. All of the following are semantically equivalent:

export class C {}

class C {}
export { C };

export { C };
class C {}

The only imperative behavior associated with export is initializing the binding that is exported.

Oops, that's right, I forgot it. Now I understand the main mistake in this topic. Thanks @rbuckton and @pzuraq for your patient and very clear contributions.

@hax
Copy link
Member Author

hax commented Feb 2, 2023

As the plenary, the consensus is allowing decorators before export XOR after export. (XOR means, @deco export class X {} and export @deco class X {} are both valid, @deco export @deco class X {} is not valid.)

Thank you everyone to contribute to this 5 years long discussion.

@hax hax closed this as completed Feb 2, 2023
@javiervelezreyes
Copy link

javiervelezreyes commented Feb 2, 2023 via email

@bathos
Copy link

bathos commented Feb 3, 2023

As the plenary, the consensus is allowing decorators before export XOR after export. (XOR means, @deco export class X {} and export @deco class X {} are both valid, @deco export @deco class X {} is not valid.)

Thank you everyone to contribute to this 5 years long discussion.

Wow. Surprise twist in the eleventh hour and it’s like ... beautiful. A rainbow bikeshed!

I raise my @ to all who fought on this storied battlefield, whether on the right side or the wrong side. Especially those who gave their lives ... you will not be forgotten. Three cheers for decorators!

@olee
Copy link

olee commented Feb 16, 2023

@hax so if I read this correctly, decorators can now go before export for classes, but the spec in README.md has not been updated yet - is that correct? It still says this:

  • Class decorators come after export and default.

@pwhissell
Copy link

There were only two options for which the community was divided; either the export keyword comes before of after the decorator. In the end the allowing both options was the best solution TC39 could have made.

It just took too long for this proposal to conclude... way too long.

@pwhissell
Copy link

It feels like there should be a post-mortem focused on how to avoid 5 years of opinionated discussion over a single proposal

@ljharb
Copy link
Member

ljharb commented Feb 23, 2023

@pwhissell most proposals are discussed for more years than that, so i'm not sure a postmortem is needed. It's just an expected part of the process for something that by and large we can never reverse (shipping something on the web, i mean)

@david0178418
Copy link

In this case, I think it would have been better to have removed exports entirely from the spec until this was sorted out. Decorators as a whole don't directly depend on this. Anyone using decorators could have just exported them on a separate line in the interim.

Perhaps future contentious, nonintegral items could be evaluated for piece-wise deferment while the bulk of a proposal goes forward?

@pzuraq
Copy link
Collaborator

pzuraq commented Feb 23, 2023

@david0178418 this was not the blocker for the majority of the time that decorators were delayed. This came up again recently, but the main issues that delayed this feature were around performance and API complexity. We went through many, many iterations of design before we found one that worked and solved all constraints.

I'm planning on writing a blog post on this when I have some time to outline all the different iterations and changes that were made for this feature over time.

@david0178418
Copy link

@pzuraq Ah, I see. Good to know. Thanks for the clarification.

@pwhissell
Copy link

Ah makes sens. I'm curious to understand more about the performance issue

robbiespeed added a commit to robbiespeed/proposal-decorators that referenced this issue Mar 2, 2023
@trusktr
Copy link
Contributor

trusktr commented Mar 12, 2023

Comment moved to new issue: #503

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 12, 2023

@trusktr this issue has been settled, please don't continue commenting on it.

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

No branches or pull requests