Skip to content

Require decoratorsBeforeExport option for decorators#8465

Merged
nicolo-ribaudo merged 3 commits intobabel:masterfrom
nicolo-ribaudo:require-decoratorsBeforeExport
Aug 15, 2018
Merged

Require decoratorsBeforeExport option for decorators#8465
nicolo-ribaudo merged 3 commits intobabel:masterfrom
nicolo-ribaudo:require-decoratorsBeforeExport

Conversation

@nicolo-ribaudo
Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo commented Aug 13, 2018

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

It has not been decided yet where decorators should go.

@nicolo-ribaudo nicolo-ribaudo added PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release Spec: Decorators labels Aug 13, 2018
@nicolo-ribaudo nicolo-ribaudo added this to the Babel 7 RC milestone Aug 13, 2018
@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented Aug 13, 2018 •
edited
Loading

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

@nicolo-ribaudo nicolo-ribaudo changed the title Require decoratorsBeforeExport option for syntax-decorators Require decoratorsBeforeExport option for decorators Aug 13, 2018
xtuc
xtuc approved these changes Aug 13, 2018
View reviewed changes
packages/babel-parser/test/plugin-options.js Outdated

it("when they aren't specified", function() {
// Currently every option is required, so this can't be tested.
it.skip("when they aren't specified", function() {
Copy link
Copy Markdown
Member

@xtuc xtuc Aug 13, 2018

Choose a reason for hiding this comment

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

Maybe we can just remove this dead code?

Copy link
Copy Markdown
Member Author

@nicolo-ribaudo nicolo-ribaudo Aug 13, 2018

Choose a reason for hiding this comment

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

I forgot about flow's all option 😆 I re-enabled the test using it.

@nicolo-ribaudo nicolo-ribaudo merged commit d79b5ee into babel:master Aug 15, 2018
@nicolo-ribaudo nicolo-ribaudo deleted the require-decoratorsBeforeExport branch August 15, 2018 07:04
@loganfsmyth loganfsmyth mentioned this pull request Aug 28, 2018
Closed
@wyuenho
Copy link
Copy Markdown

wyuenho commented Sep 9, 2018

This change makes no sense, plugin-utils.js throws as soon as decoratorsBeforeExport is undefined, but babel-plugin-syntax-decorators/src/index.js suggests that as long as legacy: true implies decoratorsBeforeExport is optional. Which exactly is the intention of this change?

@wyuenho wyuenho mentioned this pull request Sep 9, 2018
Merged
@nicolo-ribaudo
Copy link
Copy Markdown
Member Author

nicolo-ribaudo commented Sep 9, 2018 •
edited
Loading

Are you using Babel or the parser? plugin-utils.js is for the parser, while babel-plugin-syntax-decorators/src/index.js is the Babel plugin.

They have slightly different options:

Babel @babel/parser
["@babel/plugin-syntax-decorators", { "legacy": true }] "decorators-legacy"
["@babel/plugin-syntax-decorators", { "decoratorsBeforeExport": true }] ["decorators", { "decoratorsBeforeExport": true }]
["@babel/plugin-syntax-decorators", { "decoratorsBeforeExport": false }] ["decorators", { "decoratorsBeforeExport": false }]

@wyuenho
Copy link
Copy Markdown

wyuenho commented Sep 9, 2018

Ah okay thanks for the pointer. ImportJS seems to be using the parser directly

@ota-meshi ota-meshi mentioned this pull request Sep 15, 2018
Merged
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@xtuc xtuc xtuc 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 PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release Spec: Decorators

Projects

None yet

Milestone

Babel 7 RC

Development

Successfully merging this pull request may close these issues.

4 participants

@nicolo-ribaudo @babel-bot @wyuenho @xtuc