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

Update babelParse to use legacy decorator syntax #21506

Merged
merged 1 commit into from Mar 9, 2023

Conversation

valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented Mar 9, 2023

Closes #21243

What I did

Set up 'legacy-decorator' syntax for babelParser, because it is the only available plugin that supports property decorators (at least, which does not throw an error), which are heavily used in Angular.

As noted here, @babel/parser does not support registering other plugins only the ones which are integrated into the core of @babel/parser.

The legacy-decorator plugin uses the original Stage 1 proposal. Whereas e.g. Typescript uses the Stage 2 proposal (in Typescript 5.0, even the Stage 3.0 proposal).

It seems that in Babel 8, the @babel/plugin-proposal-decorators get merged into @babel/parser, which will very like support parameter decorators with the newest decorator Stage 3.0 proposal:

Add mandatory version option to the decorators plugins, and merge the two plugins in @babel/parser.

Differences between the proposals

It is essential to know in which particular areas the proposals differ from each other and whether there are any differences by applying decorators by ignoring the internal implementations of decorators, because for AST parsing this is not relevant.

Differences between Stage 2 / 3 proposal:

  • STAGE 3: ES Decorators can decorate private fields
  • STAGE 3: ES Decorators can decorate class expressions

Differences between Stage 1 / 2 proposal:

  • Only further restrictions syntax-wise. Stage 1 was richer in syntax, and Stage 2 abandoned some of it.

Differences between Typescript "experimental" decorators / Stage 2 decorators:

  • The Stage 2 proposal does not include parameter decorators

Impact evaluation:

Opting out for an older decorator proposal might introduce unforeseeable bugs. The babel parser might not be able to parse newer decorator proposal syntax and will likely throw an error.

Optional: Allow the user to overwrite the basic parsing settings

postcss-lit has implemented the possibility to overwrite the default options, passed to the babel parser. Should we do this as well?!

Alternative: Gently recover from "soft" errors.

We can pass the option errorRecovery: true to the @babel/parser to continue parsing the code, if babel can recover from the error state. Then we don't have to opt-out to the old decorators proposal and we can still parse the AST without babel throwing a non-recoverable error.

Mentionable resources

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@kasperpeulen
Copy link
Contributor

This kind of makes sense to me. Do you know if lit 2 uses the legacy proposal as well? @valentinpalkovic

@valentinpalkovic
Copy link
Contributor Author

valentinpalkovic commented Mar 9, 2023

@kasperpeulen No, they don't use it: https://github.com/43081j/postcss-lit/blob/master/src/userConfig.ts#L13.

@kasperpeulen EDIT: Sorry. I was too fast reading your comment. I will investigate what lit 2 does.

@kasperpeulen
Copy link
Contributor

@valentinpalkovic
Copy link
Contributor Author

valentinpalkovic commented Mar 9, 2023

@kasperpeulen At least in their docs they propose to use the "2018-09" version of the proposal when used with babel, which is not the legacy one. It's newer.

EDIT: Hehe, you were a couple of seconds faster. :D

@valentinpalkovic
Copy link
Contributor Author

valentinpalkovic commented Mar 9, 2023

But we have to consider that transpiling the code with babel and parsing it with @babel/parse is done in two different ways. As mentioned in the description, I just cannot configure babel plugins, which are not part of @babel/parse. That's the main issue :/

EDIT: Even the new proposals do not support parameter decorators. We have to stick with the legacy implementation for now.

@kasperpeulen
Copy link
Contributor

Right, so if I understand correctly:
The legacy-decorator syntax based on the stage 1 proposal, was richer syntax wise, so we have the higher chance it "allows" parsing other syntaxes as well, such as:

  • Typescript with "experimental" decorators
  • Babel version 2018-09
  • Babel stage-2
  • In the feature babel stage-3 and TS 5.0 decorators

Shall we include tests for lit 2.0 examples and stage 3/TS5.0 examples?

@valentinpalkovic
Copy link
Contributor Author

@kasperpeulen If we decide to stick with the current solution, I would highly recommend adding some further tests to cover more decorator syntaxes. On the other hand, it feels "wrong" to opt-out to the legacy syntax. I personally not a fan of this solution.

Copy link
Contributor

@kasperpeulen kasperpeulen left a comment

Choose a reason for hiding this comment

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

Thanks!

@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-property-decorator-usage branch from f083759 to 262b007 Compare March 9, 2023 15:33
@valentinpalkovic valentinpalkovic added the ci:daily Run the CI jobs that normally run in the daily job. label Mar 9, 2023
@valentinpalkovic valentinpalkovic merged commit 9b400e2 into next Mar 9, 2023
@valentinpalkovic valentinpalkovic deleted the valentin/fix-property-decorator-usage branch March 9, 2023 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ci:daily Run the CI jobs that normally run in the daily job.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Decorators cannot be used to decorate parameters
3 participants