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

refactor(transformer/typescript): remove all code that removes ts annotations #2992

Conversation

Dunqing
Copy link
Member

@Dunqing Dunqing commented Apr 15, 2024

As #2951 (comment) said. oxc_codegen will not print all typescript-only code. So removing ts annotations is redundant

@github-actions github-actions bot added the A-transformer Area - Transformer / Transpiler label Apr 15, 2024
Copy link
Member Author

Dunqing commented Apr 15, 2024

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

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

@Dunqing Dunqing force-pushed the 04-15-refactor_transformer_typescript_remove_all_code_that_removes_ts_annotations branch from 60861e2 to 00772cb Compare April 15, 2024 07:16
Copy link

codspeed-hq bot commented Apr 15, 2024

CodSpeed Performance Report

Merging #2992 will degrade performances by 3.26%

Comparing 04-15-refactor_transformer_typescript_remove_all_code_that_removes_ts_annotations (00772cb) with main (82e00bc)

Summary

❌ 1 regressions
✅ 29 untouched benchmarks

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

Benchmarks breakdown

Benchmark main 04-15-refactor_transformer_typescript_remove_all_code_that_removes_ts_annotations Change
semantic[antd.js] 737.9 ms 762.8 ms -3.26%

@Boshen
Copy link
Member

Boshen commented Apr 15, 2024

Wait ... this is still useful for downstream tools if they want to work with esnext ast.

Sorry for the confusion.

@Dunqing
Copy link
Member Author

Dunqing commented Apr 15, 2024

Wait ... this is still useful for downstream tools if they want to work with esnext ast.

Can you give examples of how this works downstream?

@Boshen
Copy link
Member

Boshen commented Apr 15, 2024

Wait ... this is still useful for downstream tools if they want to work with esnext ast.

Can you give examples of how this works downstream?

AST -> transformer -> (third party tools working on esnext syntax) -> codegen

@Dunqing
Copy link
Member Author

Dunqing commented Apr 15, 2024

AST -> transformer -> (third party tools working on esnext syntax) -> codegen

The third party still use oxc_codegen, so whether or not the transformer removes typescript annotation does not affect third party tools. Am I understanding right?

@Boshen
Copy link
Member

Boshen commented Apr 15, 2024

Another note: subsequent runs don't need to visit these extra nodes anymore, which can be a lot.

@Dunqing
Copy link
Member Author

Dunqing commented Apr 15, 2024

Another note: subsequent runs don't need to visit these extra nodes anymore, which can be a lot.

Yes, indeed

@Dunqing Dunqing closed this Apr 15, 2024
@Boshen Boshen deleted the 04-15-refactor_transformer_typescript_remove_all_code_that_removes_ts_annotations branch April 15, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transformer Area - Transformer / Transpiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants