Skip to content
This repository has been archived by the owner on May 19, 2018. It is now read-only.

WIP: Add new plugin (classAndFieldDecorators) for Stage-2 decorators proposal #236

Closed

Conversation

DrewML
Copy link
Member

@DrewML DrewML commented Nov 25, 2016

Q A
Bug fix? yes
Breaking change? yes
New feature? no
Deprecations? no
Spec compliancy? yes
Tests added/pass? yes
Fixed tickets #232, #231
License MIT

This is one of several PRs that will be coming in (eventually) to start updating parsing to the latest draft of the decorators proposal (Stage 2).

Proposal is here.

The biggest change of the new latest proposal introduces a new type of MemberExpression production (DecoratorMemberExpression), that does not allow computed property access ([]). This was added to address the ambiguity between a computed member expr and a computed class property.

DecoratorMemberExpression[Yield]:
    IdentifierReference[?Yield]
    DecoratorMemberExpression[?Yield].IdentifierName

TODO

  • Add hasPlugin check for classAndFieldDecorators everywhere that a check for the decorators plugin is, that is also compliant with the latest revision of the proposal.
  • Move tests for existing decorators functionality to decorators-legacy test folder
  • Disallow computed member expressions or decorated computed properties in obj literals

@loganfsmyth
Copy link
Member

Will this be breaking for cases like

class Foo {
    @decorators['foo']
    bar(){}
}

?

@DrewML
Copy link
Member Author

DrewML commented Nov 25, 2016

Will this be breaking for cases like

It should be, based on my reading of the spec (good catch, updated to breaking in the OP). Having said that, this is a case that this branch currently parses incorrectly. I'll work on fixing that. Fixed

@loganfsmyth
Copy link
Member

Breaking changes are something we need to figure out in Babel and Babylon. This kind of thing makes me wonder if we should add a decorators2016 plugin and leave the current one as-is or something, since we can't really make breaking changes to the existing logic and bumping to 7.x would have its own set of downsides.

@DrewML
Copy link
Member Author

DrewML commented Nov 26, 2016

How about classAndFieldDecorators? That way we're not tying it to a year.

The other question, though, would be what should we do in the scenario that a user has both enabled (could easily happen now that we support * for all plugins)? With your example code above, one version of the draft (the older one) would allow @decorators['foo'] to parse, but the newest draft would consider that a syntax error. Would be a bit more straight-forward without the * option, unfortunately. This is addressed by this PR

@DrewML DrewML changed the title Prevent parsing computed field names in classes as member expressions in decorator expressions WIP: Add new plugin (classAndFieldDecoratoes) for Stage-3 decorators proposal Dec 6, 2016
@DrewML DrewML changed the title WIP: Add new plugin (classAndFieldDecoratoes) for Stage-3 decorators proposal WIP: Add new plugin (classAndFieldDecorators) for Stage-3 decorators proposal Dec 6, 2016
@DrewML DrewML changed the title WIP: Add new plugin (classAndFieldDecorators) for Stage-3 decorators proposal WIP: Add new plugin (classAndFieldDecorators) for Stage-2 decorators proposal Dec 9, 2016
@danez
Copy link
Member

danez commented Jan 2, 2017

Should we make a new major version and remove the old decorator plugin and instead create the new ones? I think we also have other stuff that is breaking. node 4, deprecated plugins

@peey
Copy link
Contributor

peey commented Jun 9, 2017

I think this PR is incorrect because

  1. @decorators['foo'] is allowed
  2. It doesn't allow decorating computed properties

If my reading of the spec is correct, then

  1. Only @foo.bar.baz or @foo.bar.baz(a,b,c) syntax should be allowed where foo,bar, or baz can
    be any IdentifierName, and
  2. Decorating computed properties should be legal

@peey peey mentioned this pull request Jun 17, 2017
@hzoo
Copy link
Member

hzoo commented Jun 20, 2017

Going to close in favor of #587!

@hzoo hzoo closed this Jun 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants