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(typescript): Align isolatedDeclaration implementation with tsc #9715

Open
wants to merge 46 commits into
base: main
Choose a base branch
from

Conversation

CPunisher
Copy link
Contributor

@CPunisher CPunisher commented Nov 5, 2024

Description:

  • Basic implementation. This also includes diagnostics and type inference.
    • Function
    • Class
    • Variable declaration
    • Enum
  • Remove unused imports and declarations. Based on the result ast of fast dts transformation, we collect the usage of ids which also contain syntax context. Then we prune the ast to cut off the unused imports and declarations.
  • Port tests and bug fixes. To ensure correctness, I'd like to port the fixtures as oxc does https://github.com/oxc-project/oxc/tree/main/crates/oxc_isolated_declarations/tests/fixtures.
  • Benmarks
  • Strip internal

Implementation:
Most code is direct translation of https://github.com/oxc-project/oxc/tree/main/crates/oxc_isolated_declarations. The differences come from:

  1. swc and oxc use different ast.
  2. The transformation of swc is mutation-based, which directly mutates the whole cloned ast, while the transformation of oxc is immutation-based, which constructs and copies the ast nodes on demand. Maybe mutation-based transformation is not better, but I think it's also annoyed to construct ast nodes and it could be easy to refactor if there is some demands in the future.
  3. oxc transforms while collecting references information to prune unused idents. However, the ast nodes of swc contain syntax context, we don't need to collect the information again. So I build a reference graph and calculate the reachability of identifiers.
  4. I think oxc could have bugs. I manually compare the result with tsc --declaration --isolatedDeclarations --emitDeclarationOnly --isolatedModules --noResolve --noCheck --strictNullChecks test.ts. I also reguard those cases where tsc can't compile as undefined behaviors.

😅 Actually I find my implementation of point 2 is not too good either. I meet many bad cases after my basic implementation and write many ugly patches. So issues are welcome.

Benchmarks:
Roughly test with https://raw.githubusercontent.com/oxc-project/benchmark-files/main/vue-id.ts under M3Pro.

parse without syntax context resolution isolated declarations
swc 282.68ms 335.44ms
oxc 53.286ms 63.917ms
tsc - 3.875s

Known problem:

  • Due to the comment emitting algorithm in swc, isolated declaration may also generate comments with wrong positions.

Related issue (if exists):
closes #9705
closes #9718

Copy link

changeset-bot bot commented Nov 5, 2024

⚠️ No Changeset found

Latest commit: 6bafe04

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@CPunisher CPunisher changed the title feat(typescript): remove unused import in isolatedDeclaration feat(typescript): remove unused imports and declarations in isolatedDeclaration Nov 5, 2024
@SoonIter
Copy link

SoonIter commented Nov 5, 2024

thanks for your help❤️️

😂 got another blocking point of isolatedDeclaration

should generate the same error as tsc

src/App.tsx(8,10): error TS9007: Function must have an explicit return type annotation with --isolatedDeclarations.

I'll open another issue

Copy link

codspeed-hq bot commented Nov 5, 2024

CodSpeed Performance Report

Merging #9715 will degrade performances by 3.1%

Comparing CPunisher:feat/isolated-decl-minify (6bafe04) with main (e0fdd68)

Summary

❌ 1 regressions
✅ 193 untouched benchmarks

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

Benchmarks breakdown

Benchmark main CPunisher:feat/isolated-decl-minify Change
common/allocator/alloc/cached-no-scope/1000000 110.3 ms 113.8 ms -3.1%

@CPunisher CPunisher changed the title feat(typescript): remove unused imports and declarations in isolatedDeclaration feat(typescript): align tsc isolatedDeclaration Nov 12, 2024
@CPunisher CPunisher changed the title feat(typescript): align tsc isolatedDeclaration feat(typescript): align isolatedDeclaration implementation with tsc Nov 12, 2024
@CPunisher CPunisher changed the title feat(typescript): align isolatedDeclaration implementation with tsc feat(typescript): Align isolatedDeclaration implementation with tsc Nov 12, 2024
@CPunisher CPunisher force-pushed the feat/isolated-decl-minify branch 2 times, most recently from 49c2be0 to 533853e Compare November 13, 2024 12:10
@CPunisher CPunisher marked this pull request as ready for review November 25, 2024 03:05
@CPunisher CPunisher requested review from a team as code owners November 25, 2024 03:05
@kdy1 kdy1 added this to the Planned milestone Nov 25, 2024
@Boshen
Copy link
Contributor

Boshen commented Nov 25, 2024

FYI oxc uses immutable AST for a single transform pipeline.

@CPunisher
Copy link
Contributor Author

CPunisher commented Nov 25, 2024

FYI oxc uses immutable AST for a single transform pipeline.

I see. And I believe swc can also get better performance and other engineering benefits with immutable AST in isolated declaration transformation. After finishing, I have a little bit of regret... But maybe in the next pr 😁.

@kdy1 kdy1 self-assigned this Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants