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

fix: add decoratorAutoAccessors plugin #594

Merged
merged 3 commits into from
Jul 30, 2024

Conversation

syi0808
Copy link
Contributor

@syi0808 syi0808 commented Jul 10, 2024

add decoratorAutoAccessors plugin in typescript parser plugins to solve error.

before screenshot

Screenshot 2024-07-10 at 9 02 52 PM

after screenshot

Screenshot 2024-07-10 at 9 02 41 PM

fix: #580

Copy link

vercel bot commented Jul 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
jscodeshift ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 30, 2024 0:13am

@facebook-github-bot
Copy link

Hi @syi0808!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@Daniel15
Copy link
Member

Can you please provide an example of code that requires this option?

@ElonVolo
Copy link
Collaborator

Here's an example, @Daniel15

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-9.html

To kill two birds with one stone, I think it would be great if in addition to providing the awesome fix for this bug, syi0808 could also add a unit test(s) that make use of the accessor keyword in some example code and then makes sure the manipulation of it comes out correctly.

That way we can guard against any future regressions with the parsing.

@syi0808
Copy link
Contributor Author

syi0808 commented Jul 11, 2024

Thanks for reviews! I added test for changes now. But, before this pr e2e test didn't exist. So i add separated test for e2e. Will it be okay? @ElonVolo @Daniel15

@doouding
Copy link

I encountered this problem too. Looking forward to it.

@@ -0,0 +1,10 @@
"use strict";
Copy link
Member

Choose a reason for hiding this comment

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

Can these go in the existing src/__tests__ directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I moved the test.

const defineTest = require("../../src/testUtils").defineTest;

describe("should be parse typescript decoratorAutoAccessors correctly", function () {
defineTest(__dirname, "ts-decorator-auto-accessor", null, null, {
Copy link
Member

Choose a reason for hiding this comment

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

Please use defineInlineTest. You can also define the transform inline if you use defineInlineTest, e.g.

function transformer(file, api) {
  .....
}

defineInlineTest(transformer, .......)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review. I switched to inline test.

@Daniel15
Copy link
Member

Sorry for the delay in getting back to this. I added some comments inline.

@Daniel15
Copy link
Member

Daniel15 commented Jul 30, 2024

Thanks! I'll push a new release with this change this week some time, depending on the timing of #611.

@Daniel15 Daniel15 merged commit 36aab12 into facebook:main Jul 30, 2024
3 checks passed
defineInlineTest(
transformer,
{},
'export class Test {\n' +
Copy link
Member

Choose a reason for hiding this comment

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

This could use template strings to avoid the concatenation, but we can change it separately :)

@Daniel15
Copy link
Member

Daniel15 commented Aug 6, 2024

Released in v17.0.0. Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable parsing of accessor keyword
5 participants