SvelteKit auto-instrumentation causes faulty JSX error

kenkunz commented 1 year ago

Is there an existing issue for this?

[X] I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
[X] I have reviewed the documentation https://docs.sentry.io/
[X] I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io) as well as self-hosted (same issue occurs in both environments).

Which SDK are you using?

@sentry/sveltekit

SDK Version

7.74.1

Framework Version

1.26.0

Link to Sentry event

https://zenthropic.sentry.io/issues/4559398012/?project=4504934292258816

SDK Setup

Sentry.init({
  dsn: 'https://3365729f7ba44228b319f4ad7ae32e6a@o4504934289506304.ingest.sentry.io/4504934292258816',
  tracesSampleRate: 1.0,
});

Steps to Reproduce

I created a new minimum SvelteKit repro with Sentry to demonstrate this bug.

Create new SvelteKit project

npm create svelte@latest sentry-repro
# - Skeleton project
# - Yes, using TypeScript syntax
# - skip other add-ons

cd sentry-repro
npm i

Add Sentry using setup wizard

npx @sentry/wizard@latest -i sveltekit
# - Sentry SaaS
# - yes
# - select project

Add a src/routes/+page.ts file with a TypeScript <> cast operator

export async function load() {
 let x: unknown = 'foo';
 console.log(<string>x);
}

Start dev server and open the page
```
npm run dev
# http://localhost:5173/
```

Note the error:

Internal server error: Unterminated JSX contents. (3:22)

Modify vite.config.ts to disable auto-instrumentation:

// add sentrySvelteKit config option
autoInstrument: false

Note that the error goes away / page serves as expected

Expected Result

No error should occur at step 5.

Auto-instrumentation should not (incorrectly) interpret TypeScript cast operator <> as JSX.

Current work-around: using as cast operator instead of <>.

Actual Result

The following error occurs at step 5.

Unterminated JSX contents. (3:22)
SyntaxError: Unterminated JSX contents. (3:22)
    at constructor (/Users/ken/Code/sentry-repro/node_modules/@babel/parser/lib/index.js:356:19)
    at V8IntrinsicMixin.raise (/Users/ken/Code/sentry-repro/node_modules/@babel/parser/lib/index.js:3223:19)
    at V8IntrinsicMixin.jsxReadToken (/Users/ken/Code/sentry-repro/node_modules/@babel/parser/lib/index.js:6605:20)
    at V8IntrinsicMixin.getTokenFromCode (/Users/ken/Code/sentry-repro/node_modules/@babel/parser/lib/index.js:6947:12)
    at V8IntrinsicMixin.getTokenFromCode (/Users/ken/Code/sentry-repro/node_modules/@babel/parser/lib/index.js:9852:11)
    at V8IntrinsicMixin.nextToken (/Users/ken/Code/sentry-repro/node_modules/@babel/parser/lib/index.js:2347:10)
    at V8IntrinsicMixin.next (/Users/ken/Code/sentry-repro/node_modules/@babel/parser/lib/index.js:2258:10)
    at V8IntrinsicMixin.eat (/Users/ken/Code/sentry-repro/node_modules/@babel/parser/lib/index.js:2262:12)
    at V8IntrinsicMixin.expect (/Users/ken/Code/sentry-repro/node_modules/@babel/parser/lib/index.js:3590:10)
    at V8IntrinsicMixin.jsxParseOpeningElementAfterName (/Users/ken/Code/sentry-repro/node_modules/@babel/parser/lib/index.js:6836:10

lforst commented 1 year ago

Hi, @kenkunz thanks for reporting this! Do you mind explaining what you mean by

TypeScript <> cast operator console.log(\<string>x);

it's the first time I've ever heard of this. The newest TS version doesn't compile here either: https://www.typescriptlang.org/play?#code/MYewdgziA2CmB00QHMAUAeCAXATgSzGQD4APASgG4AoIA

Are you sure this is valid syntax?

kenkunz commented 1 year ago

Hi @lforst – thanks for looking into this.

Are you sure this is valid syntax?

Here's the documentation on Type Assertions from the official Typescript docs: https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#type-assertions

You can also use the angle-bracket syntax (except if the code is in a .tsx file), which is equivalent:
const myCanvas = <HTMLCanvasElement>document.getElementById("main_canvas");

lforst commented 1 year ago

Ah. TIL I guess we need to turn off JSX for the parser then!

cc @Lms24 I don't see any immediate option to do this in magicast, maybe we need to pass in a custom parser?

kenkunz commented 1 year ago

@Lms24 side note – I just saw your Svienna talk on the state of Sentry for Svelte (via Svelte Society YouTube channel)… nice overview… gut getan!

Lms24 commented 1 year ago

Hi @kenkunz thanks for letting us know about this!

I don't see any immediate option to do this in magicast, maybe we need to pass in a custom parser?

I think this gives us a good reason to switch to recast, providing one of the recast parsers actually supports this syntax. Gonna add this to my to do list for this week.

TIL:

This is the AST of <string>x when parsing with the typescript parser

fromaline commented 5 months ago

+1

I have the same problem right now. Luckily, I can still manually wrap the load functions.

Lms24 commented 5 months ago

Hey, sorry that this slipped through 😬 Unfortunately, I'm/we're blocked by a lot of other work at the moment so I can't give you an ETA for fixing this.

For now, @fromaline's suggestion to manually wrap load functions is probably the best workaround if you have to rely on the angle bracket style type assertions. From what I can tell, there's no difference to using as <Type> type assertions which should work fine with our auto instrumentation plugin. But obviously I don't want to force you to change your code just for Sentry. Whatever works for you best!

fromaline commented 5 months ago

@Lms24, thanks for the explanation! No worries, it's not a deal-breaker.

getsentry / sentry-javascript