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

feat(transformer): add TraverseCtx::generate_uid #3394

Merged
merged 1 commit into from
May 24, 2024

Conversation

overlookmotel
Copy link
Collaborator

@overlookmotel overlookmotel commented May 23, 2024

Add TraverseCtx::generate_uid method.

This is modelled on Babel's scope.generateUid() method. As discussed in #3251 (comment), this is required to fix most of the remaining failing tests in transformer Milestone 1.

I have implemented this to work as closely as possible to Babel, so that it will generate same output as Babel for our tests. However, as mentioned in the code comments, this means it's a pretty expensive function to call. Those code comments suggest 2 ways in which we could make it much more efficient, but we'd need to decide how we're going to handle divergence from Babel before we can decide which route to go.

I've left it as a TODO(improve-on-babel) for now.

Copy link

graphite-app bot commented May 23, 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.

Copy link
Collaborator Author

overlookmotel commented May 23, 2024

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

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

Copy link

codspeed-hq bot commented May 23, 2024

CodSpeed Performance Report

Merging #3394 will degrade performances by 4.53%

Comparing 05-23-feat_transformer_add_traversectx_generate_uid_ (d8489f4) with main (79811ca)

Summary

⚡ 1 improvements
❌ 2 regressions
✅ 24 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main 05-23-feat_transformer_add_traversectx_generate_uid_ Change
semantic[antd.js] 218.5 ms 210.2 ms +3.95%
semantic[cal.com.tsx] 55.9 ms 57.9 ms -3.37%
semantic[pdf.mjs] 34.1 ms 35.7 ms -4.53%

@overlookmotel
Copy link
Collaborator Author

The semantic benchmarks results are completely spurious. This PR does touch semantic, but only to remove some dead code.

@overlookmotel overlookmotel marked this pull request as ready for review May 23, 2024 19:23
@Dunqing
Copy link
Member

Dunqing commented May 24, 2024

Yes, the Babel implementation has a lot of room for improvement. I think once most of the plugins are done, we can move in the direction we think is efficient

panic!("Cannot generate UID");
}

fn name_is_unique(&self, name: &str) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Add comment: this goes through the whole symbol table

@Boshen Boshen merged commit bcdc658 into main May 24, 2024
27 of 28 checks passed
@Boshen Boshen deleted the 05-23-feat_transformer_add_traversectx_generate_uid_ branch May 24, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-semantic Area - Semantic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants