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(ts/fast-strip): Distinguish invalid vs unsupported #9846

Merged
merged 24 commits into from
Jan 10, 2025

Conversation

kdy1
Copy link
Member

@kdy1 kdy1 commented Jan 6, 2025

Description:

I tried adding a property to the error thrown, but wasm_bindgen does not support adding property to an error nor creating different kinds of errors.

So I throw an object with code and message instead.

@kdy1 kdy1 added this to the Planned milestone Jan 6, 2025
@kdy1 kdy1 self-assigned this Jan 6, 2025
Copy link

changeset-bot bot commented Jan 6, 2025

🦋 Changeset detected

Latest commit: 050b9ae

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

Copy link

codspeed-hq bot commented Jan 7, 2025

CodSpeed Performance Report

Merging #9846 will not alter performance

Comparing kdy1:amaro-error (5fc3dc5) with main (ae53a35)

Summary

✅ 206 untouched benchmarks

@marco-ippolito
Copy link
Contributor

marco-ippolito commented Jan 7, 2025

So when it has unsupported syntax it will have the label help: unsupported syntax.
That means I have to match with a regex which I think is not very efficient.
I'd rather not have the longer message, the error message is descriptive enough for users.
If its not possible to have it easily accessible in js I'd just remove the cause and leave it as is

@kdy1
Copy link
Member Author

kdy1 commented Jan 7, 2025

@marco-ippolito Is it fine to throw an object? I think wasm_bindgen would allow creating object with arbitrary properties.

(Not sure if it supports throwing them, though)

@marco-ippolito
Copy link
Contributor

@marco-ippolito Is it fine to throw an object? I think wasm_bindgen would allow creating object with arbitrary properties.

(Not sure if it supports throwing them, though)

I think so since we always wrap what is thrown into a Node error. Let's give it a try

@kdy1 kdy1 marked this pull request as ready for review January 8, 2025 07:08
@kdy1 kdy1 requested a review from a team as a code owner January 8, 2025 07:08
kodiakhq[bot]
kodiakhq bot previously approved these changes Jan 8, 2025
@kdy1
Copy link
Member Author

kdy1 commented Jan 8, 2025

@marco-ippolito Can you take a look on the new error output?

@marco-ippolito
Copy link
Contributor

@marco-ippolito Can you take a look on the new error output?

So I'd expect always an object with
code and message
Code could be either InvalidSyntax or UnsupportedSyntax
Message the usual error message

kodiakhq[bot]
kodiakhq bot previously approved these changes Jan 8, 2025
@kdy1
Copy link
Member Author

kdy1 commented Jan 8, 2025

Yeap, I added some test cases

kodiakhq[bot]
kodiakhq bot previously approved these changes Jan 9, 2025
kodiakhq[bot]
kodiakhq bot previously approved these changes Jan 9, 2025
Copy link

@robpalme robpalme left a comment

Choose a reason for hiding this comment

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

Thanks for adding the new tests.

My comments are non-blocking.

crates/swc_fast_ts_strip/src/lib.rs Show resolved Hide resolved
,-[5:1]
4 | class Foo {
5 | constructor(public id: string) { }
: ^^^^^^^^^^
Copy link

Choose a reason for hiding this comment

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

The ^^^ should point to the public keyword only.

@kdy1 kdy1 mentioned this pull request Jan 9, 2025
kodiakhq[bot]
kodiakhq bot previously approved these changes Jan 10, 2025
@kdy1 kdy1 enabled auto-merge (squash) January 10, 2025 02:34
@kdy1 kdy1 disabled auto-merge January 10, 2025 02:43
@kdy1 kdy1 merged commit 5709bc2 into swc-project:main Jan 10, 2025
150 of 151 checks passed
@kdy1 kdy1 deleted the amaro-error branch January 10, 2025 03:02
@marco-ippolito
Copy link
Contributor

Is this change included in the latest nightly?

@kdy1
Copy link
Member Author

kdy1 commented Jan 10, 2025

I triggered a full publish action for 1.10.7 (stable).

https://github.com/swc-project/swc/actions/runs/12703698875

@kdy1 kdy1 modified the milestones: Planned, v1.10.7 Jan 10, 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.

3 participants