Skip to content

Comments

fix(transformer): do no add __self when the jsx is inside constructor#3258

Merged
overlookmotel merged 2 commits intomainfrom
05-13-fix_transformer_do_no_add___self_when_the_jsx_is_inside_constructor
May 14, 2024
Merged

fix(transformer): do no add __self when the jsx is inside constructor#3258
overlookmotel merged 2 commits intomainfrom
05-13-fix_transformer_do_no_add___self_when_the_jsx_is_inside_constructor

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented May 13, 2024

No description provided.

@graphite-app
Copy link
Contributor

graphite-app bot commented May 13, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added the A-transformer Area - Transformer / Transpiler label May 13, 2024
@Dunqing Graphite App
Copy link
Member Author

Dunqing commented May 13, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Dunqing and the rest of your teammates on Graphite Graphite

@codspeed-hq
Copy link

codspeed-hq bot commented May 13, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #3258 will not alter performance

Comparing 05-13-fix_transformer_do_no_add___self_when_the_jsx_is_inside_constructor (1c3ac6c) with main (65540c0)

Summary

✅ 27 untouched benchmarks

@Boshen Boshen requested a review from overlookmotel May 13, 2024 16:49
@Dunqing Dunqing force-pushed the 05-13-fix_transformer_do_no_add___self_when_the_jsx_is_inside_constructor branch from a763145 to d5ea2e4 Compare May 14, 2024 04:31
@github-actions github-actions bot added the A-ast Area - AST label May 14, 2024
Dunqing
Dunqing commented May 14, 2024
View reviewed changes
@Dunqing Dunqing marked this pull request as ready for review May 14, 2024 04:34
@Dunqing Dunqing force-pushed the 05-13-fix_transformer_do_no_add___self_when_the_jsx_is_inside_constructor branch from d5ea2e4 to 50e950d Compare May 14, 2024 09:11
overlookmotel
overlookmotel reviewed May 14, 2024
View reviewed changes
Boshen
Boshen approved these changes May 14, 2024
View reviewed changes
Copy link
Member

@Boshen Boshen left a comment

Choose a reason for hiding this comment

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

Shall we merge and iterate?

@overlookmotel overlookmotel mentioned this pull request May 14, 2024
Merged
Dunqing pushed a commit that referenced this pull request May 14, 2024
@overlookmotel
fix(traverse): create scopes for functions (#3273)
6fd7a3c 
Fix bug with:

1. scopes not being created for functions.
2. too many scopes being created for class methods.

i.e. Logic for when `Function` creates a scope was opposite of what it
should be.

Discovered by @Dunqing in
#3258 (review).
@overlookmotel
Copy link
Member

overlookmotel commented May 14, 2024

Shall we merge and iterate?

I don't think we should. If we can rebase on top of #3273, then we can make sure the logic is correct before merging.

overlookmotel
overlookmotel reviewed May 14, 2024
View reviewed changes
@Dunqing Dunqing force-pushed the 05-13-fix_transformer_do_no_add___self_when_the_jsx_is_inside_constructor branch from 50e950d to 1c2892e Compare May 14, 2024 10:39
@Dunqing
Copy link
Member Author

Dunqing commented May 14, 2024

@overlookmotel Please review this again. I think we can merge first

@overlookmotel
Copy link
Member

overlookmotel commented May 14, 2024 •
edited
Loading

@overlookmotel Please review this again. I think we can merge first

@Dunqing Please see #3258 (comment) above. But if you prefer to merge this PR now, go ahead. Just please ping me and I'll look again and make a follow-up PR if necessary.

@Dunqing Dunqing mentioned this pull request May 14, 2024
Closed
@overlookmotel
Copy link
Member

overlookmotel commented May 14, 2024

@Dunqing I'm going to merge this. I do think there's still a couple of problems, but I'll do a follow-up PR.

@overlookmotel overlookmotel merged commit b4fa27a into main May 14, 2024
@overlookmotel overlookmotel deleted the 05-13-fix_transformer_do_no_add___self_when_the_jsx_is_inside_constructor branch May 14, 2024 15:12
@overlookmotel overlookmotel mentioned this pull request May 14, 2024
Closed
@overlookmotel
Copy link
Member

overlookmotel commented May 14, 2024

Turns out Dunqing's implementation was exactly correct. All my comments saying there were problems were completely wrong!

@overlookmotel overlookmotel mentioned this pull request May 14, 2024
Open
@Dunqing Dunqing mentioned this pull request May 15, 2024
Merged
Boshen pushed a commit that referenced this pull request May 15, 2024
@Dunqing
feat(transformer): do not add self attribute in react/jsx plugin (#3287)
b9d69ad 
follow-up: #3258.
@overlookmotel overlookmotel mentioned this pull request Jun 10, 2024
Merged
Boshen pushed a commit that referenced this pull request Jun 10, 2024
@overlookmotel
refactor(transformer): remove outdated comment (#3606)
3467e3d 
Remove a TODO comment where the do is done.

This was covered by #3258 and #3287.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@overlookmotel overlookmotel overlookmotel left review comments

@Boshen Boshen Boshen approved these changes

Assignees

No one assigned

Labels

A-ast Area - AST A-transformer Area - Transformer / Transpiler

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Dunqing @overlookmotel @Boshen