This repository has been archived by the owner. It is now read-only.

Decorators Stage 2 Parsing #587

Merged
merged 14 commits into from Jun 22, 2017

Conversation

@peey
Contributor

peey commented Jun 17, 2017

Q A
Bug fix? no
Breaking change? no
New feature? yes
Deprecations? no
Spec compliancy? yes
Tests added/pass? yes
Fixed tickets #13, #33
License MIT
  1. Adds a decoratorsStage2 plugin which can be used instead of decorators plugin but both cannot be used together
  2. Does not interop with class fields. This functionality will be added later when the class fields and unified class fields advance
  3. Disallows the following
    3.1. Computed key on decorator e.g. @dec[foo]
    3.2. Parameter decorators
    3.3. Object methods
    3.4. export keyword in between of class body and decorators
  4. Allows the following
    4.1 Decoration of computed class method names (#33)
    4.2 Decoration of generator functions (#13)

In some places, I've chosen to throw custom errors like "Stage 2 decorators do not allow parameter decoration" instead of throwing unexpected token so as to help people migrate and to make these changes explicit and less confusing.

Rationale for PR

The disallowed features are much loved but unfortunately either aren't a part of the proposal yet or have been discarded, therefore there is no clear answer on how to transform them.

This PR and associated plugin for stage-2 decorators is going to be opt-in for those who want to start writing what's currently stage-2 approved in the decorators proposal.

Ping @bakkot - #353; @DrewML - #236

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Jun 17, 2017

Codecov Report

Merging #587 into master will decrease coverage by 0.14%.
The diff coverage is 96.66%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #587      +/-   ##
========================================
- Coverage   98.15%    98%   -0.15%     
========================================
  Files          22     22              
  Lines        3680   3712      +32     
  Branches     1026   1036      +10     
========================================
+ Hits         3612   3638      +26     
- Misses         25     27       +2     
- Partials       43     47       +4
Flag Coverage Δ
#babel 79.87% <10%> (-0.84%) ⬇️
#babylon 96.79% <96.66%> (-0.03%) ⬇️
Impacted Files Coverage Δ
src/parser/expression.js 96.94% <100%> (+0.01%) ⬆️
src/parser/lval.js 98.06% <100%> (+0.02%) ⬆️
src/index.js 100% <100%> (ø) ⬆️
src/parser/statement.js 99.14% <95.45%> (+0.02%) ⬆️
src/parser/comments.js 85.91% <0%> (-8.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1e2c32...04b4086. Read the comment docs.

codecov bot commented Jun 17, 2017

Codecov Report

Merging #587 into master will decrease coverage by 0.14%.
The diff coverage is 96.66%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #587      +/-   ##
========================================
- Coverage   98.15%    98%   -0.15%     
========================================
  Files          22     22              
  Lines        3680   3712      +32     
  Branches     1026   1036      +10     
========================================
+ Hits         3612   3638      +26     
- Misses         25     27       +2     
- Partials       43     47       +4
Flag Coverage Δ
#babel 79.87% <10%> (-0.84%) ⬇️
#babylon 96.79% <96.66%> (-0.03%) ⬇️
Impacted Files Coverage Δ
src/parser/expression.js 96.94% <100%> (+0.01%) ⬆️
src/parser/lval.js 98.06% <100%> (+0.02%) ⬆️
src/index.js 100% <100%> (ø) ⬆️
src/parser/statement.js 99.14% <95.45%> (+0.02%) ⬆️
src/parser/comments.js 85.91% <0%> (-8.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1e2c32...04b4086. Read the comment docs.

@hzoo hzoo requested review from littledan, diervo and bakkot Jun 17, 2017

@hzoo hzoo requested a review from DrewML Jun 17, 2017

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Jun 18, 2017

Member

I don't like the decoratorsStage2 name for the plugin: what will happen when the decorators proposal reaches stage 3? I think names like decoratorsNew or decorators2 may be good candidates.

Member

nicolo-ribaudo commented Jun 18, 2017

I don't like the decoratorsStage2 name for the plugin: what will happen when the decorators proposal reaches stage 3? I think names like decoratorsNew or decorators2 may be good candidates.

@peey

This comment has been minimized.

Show comment
Hide comment
@peey

peey Jun 18, 2017

Contributor

That's a valid point. We could also name it classAndFieldDecorators like it was named in #236 if we don't mind tying it to that feature (then we'll have to put future decorator proposals like parameter decorators under a different plugin name)

Contributor

peey commented Jun 18, 2017

That's a valid point. We could also name it classAndFieldDecorators like it was named in #236 if we don't mind tying it to that feature (then we'll have to put future decorator proposals like parameter decorators under a different plugin name)

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Jun 19, 2017

Member

@nicolo-ribaudo depends on if stage 3 means more changes. Either way we would need to fork the logic again with a new plugin/name

Member

hzoo commented Jun 19, 2017

@nicolo-ribaudo depends on if stage 3 means more changes. Either way we would need to fork the logic again with a new plugin/name

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Jun 19, 2017

Member

@vjeux brought up a good point about whether the start position of the class/function part of the decorator should change? https://twitter.com/Vjeux/status/876213458622005249

Member

hzoo commented Jun 19, 2017

@vjeux brought up a good point about whether the start position of the class/function part of the decorator should change? https://twitter.com/Vjeux/status/876213458622005249

@peey

This comment has been minimized.

Show comment
Hide comment
@peey

peey Jun 19, 2017

Contributor

@hzoo Though in case stage 3 brings mostly non-breaking changes, we could easily make additions to this new decorators plugin instead of forking logic into a new one.

So we should definitely choose a plugin name that's not tied to stage 2

Contributor

peey commented Jun 19, 2017

@hzoo Though in case stage 3 brings mostly non-breaking changes, we could easily make additions to this new decorators plugin instead of forking logic into a new one.

So we should definitely choose a plugin name that's not tied to stage 2

@basicdays

This comment has been minimized.

Show comment
Hide comment
@basicdays

basicdays Jun 19, 2017

Would a test to assert the output of the following make sense?

export default @thing
class Stuff { }

basicdays commented Jun 19, 2017

Would a test to assert the output of the following make sense?

export default @thing
class Stuff { }
@peey

This comment has been minimized.

Show comment
Hide comment
@peey

peey Jun 19, 2017

Contributor
export default @thing
class Stuff { }

@basicdays yep, I think this is the preferred way to do it since decorators and class tail together make up the class, and then you can export that like you export an undecorated class

I will add a test for this

Contributor

peey commented Jun 19, 2017

export default @thing
class Stuff { }

@basicdays yep, I think this is the preferred way to do it since decorators and class tail together make up the class, and then you can export that like you export an undecorated class

I will add a test for this

Show outdated Hide outdated src/index.js
@@ -62,6 +62,10 @@ function getParserClass(pluginsFromOptions: $ReadOnlyArray<string>): Class<Parse
pluginList.unshift("estree");
}
if (pluginList.indexOf("decorators") >= 0 && pluginList.indexOf("decoratorsStage2") >= 0) {

This comment has been minimized.

@hzoo

hzoo Jun 19, 2017

Member

Don't have to do this now, but we should figure out a better solution to this in the future (regarding multiple versions of the same kind plugin). Same kind of error as TS + Flow

@hzoo

hzoo Jun 19, 2017

Member

Don't have to do this now, but we should figure out a better solution to this in the future (regarding multiple versions of the same kind plugin). Same kind of error as TS + Flow

@littledan

Huge fan of the design decisions in this patch, to follow the current draft spec, no more no less, and to make a separate flag. To bikeshed about the name, how about "standardDecorators"?

What's your long-term vision for this mode? Ultimately, it would be great if, in Babel 7.0 (or 8.0), this new syntax (and a new matching transform) would be the only decorators implementation, or at least the only one selected by the staged presets, but maybe this is too big of a breaking change.

Show outdated Hide outdated ...s/experimental/decorators-stage-2/no-computed-decorator-member/actual.js
@@ -0,0 +1,6 @@
class Foo {
@bar[bizz]

This comment has been minimized.

@littledan

littledan Jun 19, 2017

This is a great test; you could also add tests for cases like @(bar.baz) and @foo().bar, which one might expect to work. The grammar is extremely restrictive here, and you seem to implement the restrictions well (so well that I had to go back to the spec and ask, is it really that restrictive? yes).

@littledan

littledan Jun 19, 2017

This is a great test; you could also add tests for cases like @(bar.baz) and @foo().bar, which one might expect to work. The grammar is extremely restrictive here, and you seem to implement the restrictions well (so well that I had to go back to the spec and ask, is it really that restrictive? yes).

@bakkot

bakkot approved these changes Jun 19, 2017

@peey

This comment has been minimized.

Show comment
Hide comment
@peey

peey Jun 19, 2017

Contributor

Changes in latest commit

  1. Add test for exports as @basicdays suggested
  2. Add one test case for class expressions, e.g. var foo = class {}
  3. Add tests to ensure @(bar.baz) and @foo().bar fail as @littledan suggested
  4. Minor fix (use pluginsFromOptions instead of pluginList) for code that detects plugin conflict, and added a test

I think I'll do the plugin renaming in the end after everything else in the PR is looked at and approved

Contributor

peey commented Jun 19, 2017

Changes in latest commit

  1. Add test for exports as @basicdays suggested
  2. Add one test case for class expressions, e.g. var foo = class {}
  3. Add tests to ensure @(bar.baz) and @foo().bar fail as @littledan suggested
  4. Minor fix (use pluginsFromOptions instead of pluginList) for code that detects plugin conflict, and added a test

I think I'll do the plugin renaming in the end after everything else in the PR is looked at and approved

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Jun 20, 2017

Member

What's your long-term vision for this mode? Ultimately, it would be great if, in Babel 7.0 (or 8.0), this new syntax (and a new matching transform) would be the only decorators implementation, or at least the only one selected by the staged presets, but maybe this is too big of a breaking change.

Yeah we would want the preset to always be updated to the latest version (via major bumps in both the babel transform and the babel stage preset) and babylon itself would update with minor bumps via new plugin names

Member

hzoo commented Jun 20, 2017

What's your long-term vision for this mode? Ultimately, it would be great if, in Babel 7.0 (or 8.0), this new syntax (and a new matching transform) would be the only decorators implementation, or at least the only one selected by the staged presets, but maybe this is too big of a breaking change.

Yeah we would want the preset to always be updated to the latest version (via major bumps in both the babel transform and the babel stage preset) and babylon itself would update with minor bumps via new plugin names

@existentialism

👍

@peey

This comment has been minimized.

Show comment
Hide comment
@peey

peey Jun 21, 2017

Contributor

@vjeux brought up a good point about whether the start position of the class/function part of the decorator should change? https://twitter.com/Vjeux/status/876213458622005249

@vjeux trying to find the reason for this in the proposals document. Is it because the productions look like

ClassDeclaration[Yield, Default]:
    DecoratorList[?Yield]opt class BindingIdentifier[?Yield] ClassTail[?Yield]
    [+Default]DecoratorList[?Yield]opt class ClassTail[?Yield]

So we'd consider ClassDeclaration to start at the begining of the first decorator? (And similarly for class expression and methods) ??

Contributor

peey commented Jun 21, 2017

@vjeux brought up a good point about whether the start position of the class/function part of the decorator should change? https://twitter.com/Vjeux/status/876213458622005249

@vjeux trying to find the reason for this in the proposals document. Is it because the productions look like

ClassDeclaration[Yield, Default]:
    DecoratorList[?Yield]opt class BindingIdentifier[?Yield] ClassTail[?Yield]
    [+Default]DecoratorList[?Yield]opt class ClassTail[?Yield]

So we'd consider ClassDeclaration to start at the begining of the first decorator? (And similarly for class expression and methods) ??

@diervo

diervo approved these changes Jun 21, 2017

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Jun 21, 2017

All the other parent nodes include the location of the children node except for decorators. We use this property in prettier to attach comments by going down the ast looking at ranges, but it breaks for decorators. If you could fix it that would remove one hack we have to do.

vjeux commented Jun 21, 2017

All the other parent nodes include the location of the children node except for decorators. We use this property in prettier to attach comments by going down the ast looking at ranges, but it breaks for decorators. If you could fix it that would remove one hack we have to do.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Jun 21, 2017

Member

Yeah the reason is just that you'd have to backtrack to know the start of the "decorated node"

Member

hzoo commented Jun 21, 2017

Yeah the reason is just that you'd have to backtrack to know the start of the "decorated node"

@peey

This comment has been minimized.

Show comment
Hide comment
@peey

peey Jun 21, 2017

Contributor

@hzoo I've found an easy way to do it though using this.resetStartLocationFromNode method, so it won't be a problem to add this feature. Just debugging flow errors right now before I update the PR!

Contributor

peey commented Jun 21, 2017

@hzoo I've found an easy way to do it though using this.resetStartLocationFromNode method, so it won't be a problem to add this feature. Just debugging flow errors right now before I update the PR!

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Jun 21, 2017

Member

Cool didn't know about that one 😄

Member

hzoo commented Jun 21, 2017

Cool didn't know about that one 😄

@peey

This comment has been minimized.

Show comment
Hide comment
@peey

peey Jun 21, 2017

Contributor

Latest changes in last 3 commits:

  • Now all decorated elements (classes and methods) have their start location at the start location of the first decorator
  • Decide to rename to the plugin to decorators2 instead of other variants because
    1. If more backwards-incompatible changes are introduced in future and then a new plugin may be implemented as decorators3
    2. Can rename it to standardDecorators at zero cost later when the proposal is at stage 4

These two edits have caused a lot of minor changes to a lot of files so it might be a pain to review. The only logic changes to source folder is by this commit in the file src/parser/statement.js and everything else is renaming / test changes and I don't think would need review.

I also can't figure out how to increase coverage, the uncovered lines aren't making any sense to me (they existed in codebase before this PR). So if that's not a problem then I think we're good to merge (perhaps after latest changes relating to start location have been reviewed?)

Contributor

peey commented Jun 21, 2017

Latest changes in last 3 commits:

  • Now all decorated elements (classes and methods) have their start location at the start location of the first decorator
  • Decide to rename to the plugin to decorators2 instead of other variants because
    1. If more backwards-incompatible changes are introduced in future and then a new plugin may be implemented as decorators3
    2. Can rename it to standardDecorators at zero cost later when the proposal is at stage 4

These two edits have caused a lot of minor changes to a lot of files so it might be a pain to review. The only logic changes to source folder is by this commit in the file src/parser/statement.js and everything else is renaming / test changes and I don't think would need review.

I also can't figure out how to increase coverage, the uncovered lines aren't making any sense to me (they existed in codebase before this PR). So if that's not a problem then I think we're good to merge (perhaps after latest changes relating to start location have been reviewed?)

@Kovensky

👍

@@ -47,6 +47,11 @@ const parserClassCache: { [key: string]: Class<Parser> } = {};
/** Get a Parser class with plugins applied. */
function getParserClass(pluginsFromOptions: $ReadOnlyArray<string>): Class<Parser> {
if (pluginsFromOptions.indexOf("decorators") >= 0 && pluginsFromOptions.indexOf("decorators2") >= 0) {

This comment has been minimized.

@Kovensky

Kovensky Jun 22, 2017

Member

I'd say to use Array.prototype.includes instead...... but that doesn't work on Node 4 😢

@Kovensky

Kovensky Jun 22, 2017

Member

I'd say to use Array.prototype.includes instead...... but that doesn't work on Node 4 😢

@@ -0,0 +1,4 @@
{
"plugins": ["decorators2", "classProperties"],
"throws": "Stage 2 decorators may only be used with a class or a class method (2:2)"

This comment has been minimized.

@hzoo

hzoo Jun 22, 2017

Member

Should this error message mention no computed instead?

@hzoo

hzoo Jun 22, 2017

Member

Should this error message mention no computed instead?

This comment has been minimized.

@peey

peey Jun 22, 2017

Contributor

We will have to explain this in documentation or some other way.

the problem is that with classProperties enabled and due to the fact that the computed property isn't considered a part of the decorator, we can't really tell if the code is trying to access decorator's member or trying to decorate a computed class property.

So in this example it thinks that [bizz] is a class property.

If you disable class properties altogether you will get a syntax error at abc since it is expecting opening bracket right after the method name [bizz]

@peey

peey Jun 22, 2017

Contributor

We will have to explain this in documentation or some other way.

the problem is that with classProperties enabled and due to the fact that the computed property isn't considered a part of the decorator, we can't really tell if the code is trying to access decorator's member or trying to decorate a computed class property.

So in this example it thinks that [bizz] is a class property.

If you disable class properties altogether you will get a syntax error at abc since it is expecting opening bracket right after the method name [bizz]

@@ -0,0 +1,4 @@
{
"sourceType": "module",
"throws": "Leading decorators must be attached to a class declaration (2:0)"

This comment has been minimized.

@hzoo

hzoo Jun 22, 2017

Member

is it useful to say: instead of the export? or to explain it via example?

@hzoo

hzoo Jun 22, 2017

Member

is it useful to say: instead of the export? or to explain it via example?

This comment has been minimized.

@peey

peey Jun 22, 2017

Contributor

good idea

@peey

peey Jun 22, 2017

Contributor

good idea

@hzoo

hzoo approved these changes Jun 22, 2017

Looks awesome! An amazing first PR @peey, having to check the spec, and getting all these reviews!

I think maybe we can work on the error messages still if that's not difficult to change?

@hzoo hzoo merged commit f976bdd into babel:master Jun 22, 2017

1 of 3 checks passed

codecov/patch 97.05% of diff hit (target 98.15%)
Details
codecov/project 98% (-0.15%) compared to e1e2c32
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@peey peey referenced this pull request Jun 23, 2017

Merged

Follow-up on Decorators PR #590

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Jun 23, 2017

Member

Followup PR #590

Member

hzoo commented Jun 23, 2017

Followup PR #590

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