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

Require decoratorsBeforeExport option for decorators #8465

Merged
merged 3 commits into from Aug 15, 2018
Merged

Require decoratorsBeforeExport option for decorators #8465

merged 3 commits into from Aug 15, 2018

Conversation

nicolo-ribaudo
Copy link
Member

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
Collaborator

babel-bot commented Aug 13, 2018 •
edited

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
Member

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
Member Author

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

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
Member Author

nicolo-ribaudo commented Sep 9, 2018 •
edited

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

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.

None yet
4 participants
@nicolo-ribaudo @babel-bot @wyuenho @xtuc