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

Open
hax opened this Issue Mar 4, 2018 · 277 comments

Comments

Projects
None yet
@hax
Contributor

hax commented Mar 4, 2018

@deco
export class A {}

or

export @deco class A {}

or allow both?

@rbuckton

This comment has been minimized.

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

This comment has been minimized.

Collaborator

rbuckton commented Mar 4, 2018

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

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Member

littledan 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.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Member

littledan commented Mar 5, 2018

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

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

This comment has been minimized.

DanielRosenwasser commented Mar 5, 2018

@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

This comment has been minimized.

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

This comment has been minimized.

Contributor

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 tc39/proposal-decorators-previous#35 (comment) by @zenparsing
@connect(...)
export class B {
  render() {
    // ...
  }
}
  1. From tc39/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 tc39/proposal-decorators-previous#2 (comment) by @justinfagnani
@define('my-element')
export class MyElement extends HTMLElement {}

All three use @deco export class form.

@kt3k

This comment has been minimized.

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

This comment has been minimized.

loganfsmyth commented Mar 5, 2018

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

This comment has been minimized.

cztomsik commented Mar 23, 2018

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

jsg2021 commented Mar 23, 2018

The pattern I've come to is

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

This comment has been minimized.

justinfagnani commented Mar 23, 2018

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Contributor

hax commented Oct 24, 2018

@kt3k

It doesn't seem needing to keep the compatibility of the current syntax/semantics, and the users should be aware of it.

In theory, you could change everything, but...

See similar issue: tc39/proposal-class-fields#84

You will note that I used to given similar arguments like you, unfortunately...

@trotyl

This comment has been minimized.

Contributor

trotyl commented Oct 24, 2018

TypeScript will have to break compatibility anyway, since it implements an old proposal which isn't compatible with the current one.

@nicolo-ribaudo The old decorator support in TypeScript is behind an experimentalDecorators flag, so the new decorator can be enabled without any flag and it will not cause any breaking change.

But the flag name should be renamed to obsoleteDecorators possibly.

@adrianhelvik

This comment has been minimized.

adrianhelvik commented Oct 24, 2018

I have been thinking a lot about this (maybe too much). And do note that I don't care much about which variant is approved (both, none or either), as I generally don't export and decorate at the same time.

But we need consensus. I proposed not allowing either to get this through. This receiced some criticism. So I'll argue for the other option that might please everybody.

Allow both.

Point one: Export before decorator makes more sense no matter how little it is used. If it was disallowed, then return @decorated class {} or similar decorated class expressions would make less sense. (I know, I know, it is a declaration, but still. It feels logical). This proposal feels right, but will be used less in the real world. Include it because it eases newcomers into the language and follows expectations for how things should work.

Point two: Decorator before export is ergonomic and has a lot of real world usage. Want to alter the functionality of a class? Add a decorator at the line above the class. I agree that it feels wrong, but it is more useful. It is as insane as @decorator return class X {}, but it simplifies usage.

Point 3: Decorators are very useful, don't you agree? When it is standardized I will personally feel much safer about my codebase. Also, I'm looking forward to using them in node without having to rely on Babel.

So let's get off our high horses, realize that both use cases exist and bring this thing into actual non-transpiled existence!

@ljharb

This comment has been minimized.

Member

ljharb commented Oct 24, 2018

Allowing both introduces an immediate and permanent cost to linting tools, styleguides, and team discussions - shipping an unpainted bikeshed versus a painted one isn’t free.

@dantman

This comment has been minimized.

dantman commented Oct 24, 2018

By that same argument switching to decorators after export guarantees an immediate high code refactoring cost to existing codebases. (Can jscodeshift even do this kind of refactoring, which involves turning valid syntax into invalid syntax or invalid syntax to valid syntax depending on your settings?)

And also introduces the same linting/styleguide/team discussion cost. Because forcing decorators after export requires your team to decide whether to allow the export\n@deco\nclass format in your styleguide or disallow it and require late exports, and needs linting tools to enforce it.

In other words, the linting/styleguide/team discussion cost of both is no higher than the cost of forcing people to use decorators after export.

@nicolo-ribaudo

This comment has been minimized.

Contributor

nicolo-ribaudo commented Oct 24, 2018

@hax

You already confirmed Babel have supported 1 and 2, and could support 4. So even we choose 3 now and disable 1/2 in next version of Babel, the community could just keep using old versions Babel or just fork a transformer to keep using 1, 2, and add 4 (seems not very hard), plus 3 (it always possible).

If a developer explicitly choose to use a non standard feature instead of a standard one, it isn't a responsibility of the language.

And note, Babel 7 now need to support TypeScript, which means, you can't forbid 2 until TypeScript agree to break compatibility.

Yeah, we actually can. I don't know if you ever used Babel with decorators, but we have a legacy: true option which enables an old version of the proposal (at least 2 years ago, I think). Typescript currently uses that old version of the proposal.
People can use legacy: true only with decorators specifically created for that setup, which wouldn't be compatible with the current proposal even if the syntax was the same.

@dantman

(Can jscodeshift even do this kind of refactoring, which involves turning valid syntax into invalid syntax or invalid syntax to valid syntax depending on your settings?)

I don't know about jscodeshift, but Babel can (we already provide a tool to refactor from the old decorators semantics to the current one). It would be as easy as typing npx move-decorators-after-export file.js

@jsg2021

This comment has been minimized.

jsg2021 commented Oct 24, 2018

This proposal [(export before)] feels right

Add a decorator at the line above the class. I agree that it feels wrong,

We have plenty of people who feel the opposite. If you are not in the export is like return group, you will feel placing decorators in the middle of the list of modifiers odd.

@adrianhelvik

This comment has been minimized.

adrianhelvik commented Oct 24, 2018

Note that despite my feelings about that, I definitely prefer using decorators before export, and as I said, I recognize that there is more real world usage for decorators before export. My previous post argued for including both.

@hax

This comment has been minimized.

Contributor

hax commented Oct 25, 2018

@nicolo-ribaudo

If a developer explicitly choose to use a non standard feature instead of a standard one,
it isn't a responsibility of the language.

If you already foresee the consequence of your decision, then I think you can never use the excuse of "it isn't our responsibility".

@yhaskell

This comment has been minimized.

yhaskell commented Oct 25, 2018

I think having both options enabled would give people flexibility to write code in whichever way they please, and it would be still possible to disable "incorrect" place with linters.

This agrees with common practice and does not break anything.

@trusktr

This comment has been minimized.

trusktr commented Oct 25, 2018

Can someone give a good example of why we would actually want to decorate an export statement (if that were possible)? As in, what's the use case, and how would we do it without decorators, and how would it be so much more difficult doing it without decorators than with decorating an export statement?

I don't see it yet, so to me the idea of reserving @deco before export as being useful for the possible future when we can decorate an export, doesn't seem like a good one. I'd like to understand what we could actually do with export decorations.

For now, it seems better for me to be able to write @deco before or after the "export" adjective.

(I see export as an adjective, not a verb. F.e. "the export foo" is like "the red car". Is it a Noun adjunct, for example "export" being a noun that modifies another noun "const foo"?)

@jsg2021

This comment has been minimized.

jsg2021 commented Oct 25, 2018

Can someone give a good example of why we would actually want to decorate an export statement (if that were possible)? As in, what's the use case, and how would we do it without decorators, and how would it be so much more difficult doing it without decorators than with decorating an export statement?

#135 :)

@david0178418

This comment has been minimized.

david0178418 commented Nov 5, 2018

Is there movement on this as a standard? Last I saw was a vote a few of weeks ago with a slight lean towards export before decorator. If this has been no movement and this is still being hung up, I'd propose disallowing either until this gets worked out. How to export a decorated class is peripheral to the core intent of decorators. Otherwise, we're stuck in an infinite loop.

@Kukkimonsuta

This comment has been minimized.

Kukkimonsuta commented Nov 5, 2018

According to Dans poll decorator before export is more popular choice among developers. https://twitter.com/littledan/status/1050815837962158080

There is also more info in this twitter thread https://twitter.com/kennethrohde/status/1059445249188212737 about desired timeline - right now the goal seem to be getting into stage 3 in january if all goes well.

I personally hope we get decorator before export for reasons listed by rbuckton, but I'll happily use whatever as long as it ships.

@littledan

This comment has been minimized.

Member

littledan commented Nov 5, 2018

The big thing that I got out of that poll is, there is support in the community for all sorts of positions, and a lot of it: Lots of people would actually like export before decorators. This is not just a "theorist" position. There's also people in the community who prefer longer and shorter periods of deliberation. The splits within this thread and within TC39 reflect real splits within the JS community. I hope that January can be a nice moderate amount of time to deliberate this issue.

@cscott

This comment has been minimized.

cscott commented Dec 4, 2018

FWIW, as a latecomer to this discussion, I like the mental simplification that export (and abstract, and private, and static, and ...) are just "special types of built-in decorators", and therefore I like a version of "option 4" which allows the export keyword anywhere in the list of decorators, either before or after or in the middle.

I know this isn't "really" how class keywords work, but it's a trick that allows for a simplified mental model. Any other choice is bound to frustrate/surprise some hapless newcomer to this discussion who doesn't understand your personal preferred rationale for why one position or the other is "obviously the only logical place". And if you think of export as just a special sort of decorator, then applying a decorator "before" or "after" the export "happens" makes a kind of obvious sense, and would even permit the sorts of shenanigans in #135, if it came to that.

@isiahmeadows

This comment has been minimized.

isiahmeadows commented Dec 4, 2018

@cscott I actually like that idea of treating things like export, export default, and static as kind of special "keyword decorators" in a way. I know it'd result in some potentially weird-looking semantics, but I'm not convinced it'd really restrict future additions - really, it'd make room for them. It'd also provide a clearer syntactic story for things like decorating functions and object properties.

I'd draw the line on things like function, async, get, and set - those are more like normal tokens than "decorators". Those aren't simple modifiers, but they control how the value is parsed and generated in the first place.


For a couple examples:

// Allowed
@allows("https://myapp.doodad.com")
export @static class A { ... }

// Not allowed
@allows("https://myapp.doodad.com")
export class @static A { ... }

// Allowed
@uses("dependency")
static @memoize async foo() { ... }

// Not allowed
@uses("dependency")
static async @memoize foo() { ... }

// Allowed
@slot("name")
@lazy get bar() { ... }

// Not allowed
@slot("name")
get @lazy bar() { ... }
@cscott

This comment has been minimized.

cscott commented Dec 4, 2018

@isiahmeadows Yup, works for me. IMO this actually simplifies the parsing productions a lot; the keyword modifiers are kind of a mess combinatorially if you try to limit the order too much. Easier just to allow any "non-token-like" (as you say) modifier anywhere then sort out ordering issues (in the rare case they are needed) in a later semantic checking pass.

For a "#3" like compromise, you could add the flexible grammar to the proposal, but then just note that the presence of both export and decorators simultaneously in the modifier list is prohibited (for now) by a semantic check. Then you could work out something like #135 (comment) before removing that note and allowing export and decorators to (potentially) interact.

@gibson042

This comment has been minimized.

gibson042 commented Dec 4, 2018

Would you then limit pre-export decorators to a subset of the export syntax?

@alterClass
export { uncertain };
let uncertain = Math.random() < 0.5 ? null : class {};
@alterClass
export default (Math.random() < 0.5 ? null : class {});
@alterClass
export { uncertain } from "elsewhere";
@alterSingleValue
export { A, B };
let A = class {}, B = class {};
@markExported
export let decoratedɁ = class {};
if ( isExported(decoratedɁ) ) { … }
@isiahmeadows

This comment has been minimized.

isiahmeadows commented Dec 4, 2018

@gibson042 That looks more like it'd belong in #135.

@gibson042

This comment has been minimized.

gibson042 commented Dec 5, 2018

No, it is here intentionally. Allowing @classDecorator export … would have consequences on the grammar, making things unnecessarily more difficult for both implementers and users of ECMAScript. Much of this conversation seems to ignore syntax other than export [default] class, risking hazards of the sort pointed out by @waldemarhorwat.

@LandonSchropp

This comment has been minimized.

LandonSchropp commented Dec 5, 2018

I'd like to throw in my current real-world case. I'm using decorators with MobX and React. This is a common pattern in my codebase.

@inject('awesomeStore')
@observer
export default class Awesome extends React.Component {
  ...
}

The alternative order feels really funky to me.

export default
@inject('awesomeStore')
@observer
class Awesome extends React.Component {
  ...
}

I don't like exporting my class at the end of the file because it's really easy to forget to do.

@inject('awesomeStore')
@observer
class Awesome extends React.Component {
  ...
}

export default Awesome;

In this use case, I strongly prefer to use decorators before the export key.

@isiahmeadows

This comment has been minimized.

isiahmeadows commented Dec 6, 2018

@gibson042

No, it is here intentionally. Allowing @classDecorator export … would have consequences on the grammar, making things unnecessarily more difficult for both implementers and users of ECMAScript. Much of this conversation seems to ignore syntax other than export [default] class, risking hazards of the sort pointed out by @waldemarhorwat.

I'm failing to see the slippery slope here - you could handle it consistently without adding the grammatically ambiguous mess. But also, the stuff like export/export default makes for a good shorthand of how this would look and work in practice - most people would expect this to work similarly with other modifiers like static.

@yhaskell

This comment has been minimized.

yhaskell commented Dec 6, 2018

@gibson042

As a solution we can on a parser level allow to decorate more statements (not just ClassDeclaration); then during lexical analysis we do something like

DecoratedStatement(decorator, ExportDeclaration(t)) =>  ExportDeclaration(DecoratedStatement(decorator, t))

Yes, it presents a small issue ofln a grammar level, but it also allow us to think more and maybe allow to decorate more things in the future.

@gibson042

This comment has been minimized.

gibson042 commented Dec 6, 2018

I'm not making a slippery slope argument, just pointing out the inherent complexity of what is being suggested. You may think of export as a "keyword decorator", but it actually is a statement-initiating keyword—or at least, statement-initiating unless this lands—and note that there is no class to be found in e.g. export default Object.freeze([]).

The complexity for implementers would come from introducing new early errors to export forms other than export [default] class (e.g., @classDecorator export let base = 10 would be allowed by the grammar but rejected as invalid syntax before evaluation). There is probably also an issue around the ModuleItem and/or ExportDeclaration productions, but it's possible there's a trick to keep them simple and avoid arbitrary-length lookahead in parsers.

The complexity for users would come from having to comprehend even more subtle differences with dramatic effects (e.g., @classDecorator export default class {} would be valid but @classDecorator export default (class {}) would not, despite both forms being valid in the absence of a decorator).

@cscott

This comment has been minimized.

cscott commented Dec 6, 2018

@gibson042 Isn't the fundamental issue here that we have export <expression> and export <declaration> (with the latter possibly being <declaration-including-export-somewhere>)?
That seems to argue for the "export before decorator" syntax, so that grammatically we have export (<expression>|<declaration>) instead of two wildly different syntactic forms.

But basically I believe the grammar already differentiates between a "class declaration" and a "class expression", and differentiates between an "export expression" and an "export declaration", so the before-or-after debate isn't really adding any new grammatical categories.

I think we've basically reached the end of the debate-in-the-absence-of-code phase, and what's needed to move the discussion forward are some concrete grammar diffs illustrating the different options.

@isiahmeadows

This comment has been minimized.

isiahmeadows commented Dec 7, 2018

@gibson042

I'm not making a slippery slope argument, just pointing out the inherent complexity of what is being suggested. You may think of export as a "keyword decorator", but it actually is a statement-initiating keyword—or at least, statement-initiating unless this lands—and note that there is no class to be found in e.g. export default Object.freeze([]).

I'm not talking semantics, but high level concepts, and I was explicitly reframing it in a different light for classes specifically.

The complexity for implementers would come from introducing new early errors to export forms other than export [default] class (e.g., @classDecorator export let base = 10 would be allowed by the grammar but rejected as invalid syntax before evaluation). There is probably also an issue around the ModuleItem and/or ExportDeclaration productions, but it's possible there's a trick to keep them simple and avoid arbitrary-length lookahead in parsers.

Have you actually tried implementing this? I've written a few parsers and have worked on JS parsers, too. This isn't as complicated as you're making it seem, and yes, there are tricks (really, the usual parser tricks apply here). It's certainly not the mess of parenthesized arrow functions, where the => is what makes the LHS an arguments list and not a sequence expression, two completely different grammars that are incredibly difficult to differentiate without arbitrary-length buffering.

The complexity for users would come from having to comprehend even more subtle differences with dramatic effects (e.g., @classDecorator export default class {} would be valid but @classDecorator export default (class {}) would not, despite both forms being valid in the absence of a decorator).

The key difference here is the parentheses, which aren't exactly subtle. People are already used to parentheses having dramatic effects on how a statement is read, since (a + b) * c and a + (b * c) return two completely different results. There's also export default function foo() {} vs export default (function foo() {}), where the first declares a foo binding in the outer scope, but the second does not. I don't see how users would be confused about the fact parentheses change grammar or semantics in yet another case.

@rbuckton

This comment has been minimized.

Collaborator

rbuckton commented Dec 7, 2018

The complexity for implementers would come from introducing new early errors to export forms other than export [default] class (e.g., @classDecorator export let base = 10 would be allowed by the grammar but rejected as invalid syntax before evaluation). There is probably also an issue around the ModuleItem and/or ExportDeclaration productions, but it's possible there's a trick to keep them simple and avoid arbitrary-length lookahead in parsers.

Have you actually tried implementing this? I've written a few parsers and have worked on JS parsers, too. This isn't as complicated as you're making it seem, and yes, there are tricks (really, the usual parser tricks apply here). It's certainly not the mess of parenthesized arrow functions, where the => is what makes the LHS an arguments list and not a sequence expression, two completely different grammars that are incredibly difficult to differentiate without arbitrary-length buffering.

I concur with @isiahmeadows, this isn't actually that complicated. In fact, we already merged the necessary changes into the proposal at one point (though that change was later reverted while we are still in the process of making a final determination): #95

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