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.

Sign up for GitHub

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

Jump to bottom

[legacy decorators] Allow decorating generator methods #9912

Merged
merged 2 commits into from Jun 30, 2019
Merged

[legacy decorators] Allow decorating generator methods #9912

merged 2 commits into from Jun 30, 2019

Conversation

nicolo-ribaudo
Copy link
Member

The old proposal used LeftHandSideExpression (instead of
AssignmentExpression) to satisfy this usecase:
wycats/javascript-decorators@e240cbc

Q                       A
Fixed Issues? Fixes #9852
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This PR will break the code of people doing things like

class A {
  @foo && bar
  method() {}
}

I don't think that it is a problem because:

  1. I have never seen that pattern anywhere
  2. Prettier inserts parentheses, which still work (@(foo && bar))
  3. The "legacy" proposal disallows that code.

@nicolo-ribaudo
[legacy decorators] Allow decorating generator methods
280e2ff 
The old proposal used LeftHandSideExpression (instead of
AssignmentExpression) to satisfy this usecase:
wycats/javascript-decorators@e240cbc
@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: parser Spec: Decorators (Legacy) labels Apr 26, 2019
@babel-bot
Copy link
Collaborator

babel-bot commented Apr 26, 2019 •
edited

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10739/

@babel-bot
Copy link
Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10738/

NullVoxPopuli
NullVoxPopuli reviewed Apr 27, 2019
View reviewed changes
packages/babel-parser/test/fixtures/experimental/decorators/method-generator-decorator/input.js
@@ -0,0 +1,4 @@
class Foo {
@deco
*generatorMethod() {}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YAAAAAS. This'll make ember-concurrency beautiful!

@buschtoens buschtoens mentioned this pull request Apr 30, 2019
Closed
@danez
Copy link
Member

danez commented May 3, 2019 •
edited

This PR will break the code of people doing things like

class A {
  @foo && bar
  method() {}
}

Just to understand this correctly: it wasn't allowed in legacy decorators, but it was in the old "new" implementation, but now in the "newest" it is disallowed?

@nicolo-ribaudo
Copy link
Member Author

It was allowed in a very early version of legacy decorators. It is disallowed in the "last" version of legacy decorators, in the "old new (a few months ago)" implementation and in the new not-yet-implemented version. Other than that, it is also disallowed by Flow and TypeScript.

@robclancy
Copy link

Is this going to be in the next release? It's a pretty big issue requiring ugly workarounds if needing decorators for generators. A lot of people are waiting on this.

@nicolo-ribaudo nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories and removed PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels May 14, 2019
@nicolo-ribaudo nicolo-ribaudo added this to the v7.5.0 milestone May 14, 2019
@robclancy robclancy mentioned this pull request Jun 3, 2019
Closed
buschtoens added a commit to machty/ember-concurrency-decorators that referenced this pull request Jun 4, 2019
@buschtoens
chore(deps): patch @babel/parser
fac7aba 
babel/babel#9912
@buschtoens buschtoens mentioned this pull request Jun 4, 2019
Merged
buschtoens added a commit to machty/ember-concurrency-decorators that referenced this pull request Jun 4, 2019
@buschtoens
chore(deps): patch @babel/parser
231e73b 
babel/babel#9912
buschtoens added a commit to machty/ember-concurrency-decorators that referenced this pull request Jun 4, 2019
@buschtoens
chore(deps): patch @babel/parser
b14b0bf 
babel/babel#9912
hzoo
hzoo approved these changes Jun 12, 2019
View reviewed changes
Copy link
Member

@hzoo hzoo left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice fix!

maybe also document this on the docs page?

@nicolo-ribaudo nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Jun 12, 2019
@nicolo-ribaudo nicolo-ribaudo merged commit 8bf9714 into babel:master Jun 30, 2019
@nicolo-ribaudo nicolo-ribaudo deleted the legacy-decorators/01-generator-method branch June 30, 2019 09:48
@snyk-bot snyk-bot mentioned this pull request Aug 20, 2019
Closed
@snyk-bot snyk-bot mentioned this pull request Sep 23, 2019
Open
@snyk-bot snyk-bot mentioned this pull request Oct 3, 2019
Open
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@NullVoxPopuli NullVoxPopuli NullVoxPopuli left review comments

@existentialism existentialism existentialism approved these changes

@Alonski Alonski Alonski approved these changes

@hzoo hzoo hzoo approved these changes

Assignees
No one assigned
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: New Feature 🚀 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release Spec: Decorators (Legacy)
Projects
None yet
Milestone
v7.5.0
Development

Successfully merging this pull request may close these issues.

Legacy Decorators + Generator Methods
8 participants
@nicolo-ribaudo @babel-bot @danez @robclancy @existentialism @NullVoxPopuli @Alonski @hzoo