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(ts/fast-strip): Handle unsupported module keyword #10022

Merged
merged 7 commits into from
Feb 13, 2025

Conversation

magic-akari
Copy link
Member

@magic-akari magic-akari commented Feb 11, 2025

@magic-akari magic-akari requested a review from a team as a code owner February 11, 2025 07:45
Copy link

changeset-bot bot commented Feb 11, 2025

🦋 Changeset detected

Latest commit: 987bc96

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

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

@magic-akari
Copy link
Member Author

The implementation is currently in type-strip as it allows us to directly access tokens and source code text, minimizing the risk of AST breaking changes.

It’s unclear whether we can achieve the same in the Transform pass, but it is something we need to try.

@kdy Please let me know your thoughts. Thank you!

@kdy1 kdy1 added this to the Planned milestone Feb 11, 2025
@kdy1 kdy1 self-assigned this Feb 11, 2025
Copy link

codspeed-hq bot commented Feb 11, 2025

CodSpeed Performance Report

Merging #10022 will degrade performances by 4.49%

Comparing magic-akari:es/ts-error-on-module (987bc96) with main (6dab49a)

Summary

⚡ 1 improvements
❌ 2 regressions
✅ 193 untouched benchmarks

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

Benchmarks breakdown

Benchmark BASE HEAD Change
es/minifier/libs/jquery 127.6 ms 133.6 ms -4.49%
es/minifier/libs/lodash 148.5 ms 155.4 ms -4.47%
typescript/fast-strip 767.5 µs 679.7 µs +12.92%

@kdy1
Copy link
Member

kdy1 commented Feb 11, 2025

I think I need to make a AST-breaking change tomorrow, so please feel free to work on it. Please tell me if you prefer to make the change with another PR

@magic-akari
Copy link
Member Author

I've found a solution that preserves our current AST structure and avoids introducing any breaking changes. It's straightforward to pass the source text to the Transform pass.

However, if we decide to update the AST, we might consider adding a field to specify the module or namespace. This could also benefit other libraries (such as dprint), though I'm not completely certain about its overall impact.

Which do you prefer?

@kdy1
Copy link
Member

kdy1 commented Feb 11, 2025

I think the latter one is better, but it would be better if AST-breaking change is a separate PR. If it's the case I can align the date of merging with release date of various tools to reduce impact of the breaking changes.

@magic-akari
Copy link
Member Author

I’ve created PR #10023. Please note that this PR is independent of #10023. Once #10023 is merged, we can simplify the namespace detection method accordingly.

@magic-akari magic-akari marked this pull request as draft February 11, 2025 14:44
@magic-akari magic-akari force-pushed the es/ts-error-on-module branch from b0ada55 to 48bea07 Compare February 11, 2025 16:11
@magic-akari magic-akari changed the title fix(es/ts_strip): Handle unsupported module keyword in strip-only mode fix(es/ts_strip): Handle unsupported module keyword Feb 11, 2025
@magic-akari magic-akari marked this pull request as ready for review February 11, 2025 17:05
@magic-akari magic-akari marked this pull request as draft February 11, 2025 17:58
@magic-akari
Copy link
Member Author

This is a bit awkward. We could have simply removed the top-level namespace, but we had to dig into the internal nodes to catch the error.

namespace Foo {
    namespace Bar {
        module Boom { }
    }
}

@magic-akari magic-akari marked this pull request as ready for review February 11, 2025 18:35
@kdy1 kdy1 changed the title fix(es/ts_strip): Handle unsupported module keyword fix(ts/fast-strip): Handle unsupported module keyword Feb 12, 2025
@kdy1 kdy1 merged commit 308f5d0 into swc-project:main Feb 13, 2025
18 checks passed
@magic-akari magic-akari deleted the es/ts-error-on-module branch February 13, 2025 01:32
@kdy1 kdy1 removed this from the Planned milestone Feb 18, 2025
@kdy1 kdy1 added this to the v1.10.17 milestone Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Option to error on module namespaces
2 participants