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.

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

Add an option to Babylon to have decorators before export #7869

Merged
merged 3 commits into from
May 15, 2018

Conversation

nicolo-ribaudo
Copy link
Member

Q                       A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? 👍
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT
  • 8f76ff3 adds support for options to Babylon plugins.

  • 3c5c47f adds support for decorators before export (via ["decorators2", { "decoratorsBeforeExport": true }]). I think that this should be an option because the discussion about it is still open; this would allow us to experiment both syntaxes. We can discuss about what the default value is.

@babel-bot
Copy link
Collaborator

babel-bot commented May 4, 2018

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

They work similarly to how they work in Babel. e.g.
  babylon.parse({
    options: [
      "plugin1",
      ["plugin2", { option: true }]
    ]
  });

The inernal api to get an option is
  this.getPluginOption("pluginName", "option")
If the plugin isn't defined, it returns undefined.
@nicolo-ribaudo nicolo-ribaudo force-pushed the decorators-parser-options branch 2 times, most recently from 6860e64 to 1df9568 Compare May 11, 2018 14:18
@nicolo-ribaudo
Copy link
Member Author

I think that after this PR I will need to open only one more. If we can finish the work on decorators before the TC39 meeting it would be super!

Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

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

Just a small nit, but LGTM!

) {
throw new Error("Cannot use decorators and decorators2 plugin together");
throw new Error(
"Cannot use decorators and decorators-legacy plugin together",
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

Cannot use the decorators and decorators-legacy plugins together.

@existentialism existentialism merged commit e45d5c3 into babel:master May 15, 2018
@nicolo-ribaudo nicolo-ribaudo deleted the decorators-parser-options branch May 15, 2018 16:40
p => p === "estree" || p === "flow" || p === "jsx" || p === "typescript",
);
let pluginList = pluginsFromOptions.filter(plugin => {
const p = getPluginName(plugin);
Copy link
Member

Choose a reason for hiding this comment

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

Hey all, I'm concerned about this. Now that we've defined the ability to pass options to Babylon plugins, ordering becomes critical. What if some other piece of tooling injected a syntax plugin into the plugin list after this one, or before? Which one should take priority? For instance for https://github.com/babel/babel/pull/7934/files#diff-3a8233bcd2766d2c7d87f23f944f7726R12, say the user's framework auto-enables flow, but the user wants to enable all, which comes first?

So far we've avoided this problem by not having options, and not worrying about duplicates. Now it will matter very much both what order your Babel plugins are in, but also what order they enable Babylon plugins in.

At a minimum, have we verified that the ordering on these is consistent with the ordering of visitors? e.g. If two plugins are enabled with different options, whichever one's visitors run first, is also the one who's Babylon configuration takes affect?

Copy link
Member

Choose a reason for hiding this comment

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

Let's continue the discussion over in #7984 instead of here.

this.hasPlugin("decorators") &&
this.getPluginOption("decorators", "decoratorsBeforeExport")
) {
this.unexpected();
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to have these throw more helpful errors about what the user is supposed to do to get their code to parse.

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 Spec: Decorators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants