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

Parse parameter decorators #12

Merged
merged 1 commit into from Mar 28, 2016

Conversation

Projects
None yet
5 participants
@shuhei
Copy link
Member

shuhei commented Mar 21, 2016

Because Method Parameter Decorators is now a TC39 stage 0 proposal, I'd like to propose babylon to parse parameter decorators.

While the actual transform is still being debated, it would be great if babylon only parses the syntax and allows users to transform it with third-party plugins. One concrete use case is Angular 2's decorators.

https://phabricator.babeljs.io/T1301

(Reopening #3 that was automatically closed because of rebased master)

"name": "x",
"decorators": [
{
"type": "Decorator",

This comment has been minimized.

@shuhei

shuhei Mar 21, 2016

Author Member

@thejameskyle commented here:

Decorator should probably be broken into an alias of many different types.

interface Decorator <: Node {
 expression: Expression;
}

interface ClassDecorator <: Decorator {
 type: "ClassDecorator"
}

interface ParameterDecorator <: Decorator {
 type: "ParameterDecorator"
}

// etc...

This comment has been minimized.

@shuhei

shuhei Mar 21, 2016

Author Member

@thejameskyle Thanks for your comment!

I agree that different types of decorators should be renamed and have Decorator as an alias to allow transformers to exactly match desired nodes. However, as far as I looked through babel-types, there are already 6 different types of decorators that are all expressed as Decorator:

  • ObjectMethod
  • ObjectProperty
  • ClassDeclaration
  • ClassExpression
  • ClassMethod
  • ClassProperty

I think the renaming can be done separately from this PR because it will involve a lot of changes to other packages like babel-types. What do you think?

This comment has been minimized.

@jamiebuilds

jamiebuilds Mar 21, 2016

Contributor

I'm worried how this will impact existing transforms. Adding a Decorator type in a new location will cause transforms that are looking for existing types of decorators without validating the rest of the AST will break.

Maybe this one can be the first Decorator node of a new sub-type and we can update the rest later?

@hzoo hzoo added the i: enhancement label Mar 21, 2016

@shuhei shuhei force-pushed the shuhei:parameter-decorators branch 2 times, most recently from 82943ad to 87edfd1 Mar 22, 2016

@shuhei

This comment has been minimized.

Copy link
Member Author

shuhei commented Mar 22, 2016

@thejameskyle I've updated the code to use ParameterDecorator type as you suggested.

Also, I've excluded function parameter decorators because the proposal says:

Function expression and function declaration parameters

This proposal currently targets only method parameters, because function expressions and declarations don't currently have a proposal that would be in Stage 0 or higher.

@IgorMinar

This comment has been minimized.

Copy link

IgorMinar commented Mar 22, 2016

EDITED: I mistakenly wrote "function expression" when I meant "function declaration". it's now corrected.

Function declaration decorators and function parameter decorators have been
excluded from the proposal due to semantical issues (primarily the temporal
dead zone issues).

It wouldn't hurt to have syntactical support for these two so that we can
experiment with various implementations to get the semantics right.

I do expect that over time, especially if the implementation based on
mirrors is picked as the winner, function declaration decorators and
function parameter decorators will be included in the proposals.

On Tue, Mar 22, 2016 at 8:52 AM Shuhei Kagawa notifications@github.com
wrote:

@thejameskyle https://github.com/thejameskyle I've updated the code to
use ParameterDecorator type as you suggested.

Also, I've excluded function parameter decorators because the proposal
https://docs.google.com/document/d/1Qpkqf_8NzAwfD8LdnqPjXAQ2wwh8BBUGynhn-ZlCWT0/edit#
says:

Function expression and function declaration parameters

This proposal currently targets only method parameters, because function
expressions and declarations don't currently have a proposal that would be
in Stage 0 or higher.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#12 (comment)

@shuhei

This comment has been minimized.

Copy link
Member Author

shuhei commented Mar 23, 2016

@IgorMinar I'd like to parse function parameter decorators too but I'd rather want this PR to be merged sooner.

@thejameskyle What do you think about supporting function parameters decorators at this time?

@IgorMinar

This comment has been minimized.

Copy link

IgorMinar commented Mar 23, 2016

I'm fine with either. If excluding function declaration and function
parameter decorators makes it easier to get this merged then go ahead
without those two. We primarily care about function expression decorators
and method parameter decorators.

On Wed, Mar 23, 2016 at 4:21 AM Shuhei Kagawa notifications@github.com
wrote:

@IgorMinar https://github.com/IgorMinar I'd like to parse function
parameter decorators too but I'd rather want this PR to be merged sooner.

@thejameskyle https://github.com/thejameskyle What do you think about
supporting function parameters decorators at this time?


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#12 (comment)

@jamiebuilds

This comment has been minimized.

Copy link
Contributor

jamiebuilds commented Mar 23, 2016

It's probably fine if they are be behind separate this.hasPlugin("...") checks, which the work in this PR should already be behind.

@kittens

This comment has been minimized.

Copy link
Contributor

kittens commented Mar 23, 2016

Sorry I haven't had a chance to look at this sooner. This looks good besides the fact that ParameterDecorator should just be Decorator. We already overload this node type for class decorators as well as method ones. We can look into splitting it up in a major but I'd prefer we stick to just Decorator for consistency.

@shuhei shuhei force-pushed the shuhei:parameter-decorators branch from 87edfd1 to 066229f Mar 24, 2016

@shuhei

This comment has been minimized.

Copy link
Member Author

shuhei commented Mar 24, 2016

@kittens @thejameskyle Updated ParameterDecorator to Decorator and dropped the distinction between method and function parameters. Please let me know if there's anything to be done before it's merged.

@IgorMinar Just to make it clear, function expression decorators are currently not supported due to this check.

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Mar 24, 2016

Don't know if we want a test for params in other node type that are functions: ArrowFunctionExpression, ExportDefaultDeclaration?

method-parameter-decorator is ClassMethod in babylon, and there's also ObjectMethod.

@shuhei

This comment has been minimized.

Copy link
Member Author

shuhei commented Mar 25, 2016

@hzoo Thanks for your advice. I'll add tests for the other node types.

@shuhei shuhei force-pushed the shuhei:parameter-decorators branch from 066229f to 29a6578 Mar 25, 2016

@shuhei

This comment has been minimized.

Copy link
Member Author

shuhei commented Mar 25, 2016

@hzoo I added tests for other node types except ArrowFunctionExpression. It does not work and I'd like to leave it as is for now.

ArrowFunctionExpression's parameter decorator throws "Leading decorators must be attached to a class declaration" error in parseDecorators with this PR's implementation. ArrowFunctionExpression's parameter is parsed by parseParenAndDistinguishExpression (with parens) or parseExprAtom (without parens) without knowing that it is a parameter until => appears. In the parse methods, parseDecorators throws the error because the parameter is not a class. The other node types' work because they are parsed by parseBindingList that doesn't use parseDecorators.

To support ArrowFunctionExpression's parameter decorators, we have to strip the class-only check in parseDecorators or parse ArrowFunctionExpression's parameters differently somehow. Both ways might affect the current behavior of Babylon and I don't think they are worth doing while ArrowFunctionExpression's parameter decorator is not actually in the stage 0 proposal yet.

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Mar 25, 2016

Sounds good, I didn't think arrow functions were in there but I didn't check the proposal or anything 😛.

@IgorMinar

This comment has been minimized.

Copy link

IgorMinar commented Mar 28, 2016

@hzoo @thejameskyle is this PR good? Can we get it merged? thanks!

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Mar 28, 2016

👍 from me

@kittens kittens merged commit 8b15081 into babel:master Mar 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Mar 28, 2016

@kittens

We may want to figure out the issue with runtime 5/6 before we release I think? And we haven't made a release yet since moving the repo

@shuhei

This comment has been minimized.

Copy link
Member Author

shuhei commented Mar 29, 2016

Thanks everyone 🎉

@hzoo Do we need to update other packages before releasing it? I'm afraid that a new property decorators on existing node types may be caught as an error by node type/property check.

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Mar 29, 2016

Yeah we'll need to update babel-types at least https://github.com/babel/babel/tree/master/packages/babel-types/src/definitions, but the checks are restrictive checks so it shouldn't error? (Benefits of keeping it in the monorepo are showing 😄).

The decorators transform isn't working atm and there's logan's https://github.com/loganfsmyth/babel-plugin-transform-decorators-legacy

@shuhei

This comment has been minimized.

Copy link
Member Author

shuhei commented Mar 29, 2016

@hzoo Thanks! As far as I know, the types that will get decorators as a field will be:

  • Identifier
  • AssignmentPattern
  • ArrayPattern
  • ObjectPattern
  • RestElement

The legacy transformer shouldn't error because it doesn't use Decorator visitor.

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