chore(svelte): Detect and report SvelteKit usage #5594
Conversation
size-limit report 📦
|
use async to detect sveltekit and add tests switch to using global evt processor instead of async approach
Lms24
force-pushed
the
lms-svelte-detect-sveltekit
branch
from
21f06cf
to
ec0b276
Compare
Lms24
changed the title
| detectedSvelteKit = isSvelteKitApp(); | ||
| } | ||
| if (detectedSvelteKit) { | ||
| event.modules = { |
There was a problem hiding this comment.
Where does event.modules show up? Is it something that is indexed and we can query for?
There was a problem hiding this comment.
Yup - it displays in event details page, and is indexed - we can query for it in looker.
packages/svelte/src/sdk.ts
Outdated
| } | ||
| if (detectedSvelteKit) { | ||
| event.modules = { | ||
| svelteKit: '1.0', |
There was a problem hiding this comment.
Hmm - feels weird to write 1.0 as that might not be the correct version. Thoughts?
There was a problem hiding this comment.
I agree, it's weird but we have no way of knowing which SvelteKit version was actually being used (let alone that 1.0 isn't even stable yet). So my train of thought for 1.0 was to simply put there something because afaik we have to supply a version here. But happy to change it to 1.x, 0.x, 0 or whatever. Or maybe something like unknown/n/a ? Though I'm not sure which values are allowed here (for example, how they are indexed).
There was a problem hiding this comment.
Until we are able to confidently discover correct SvelteKit version I think we can keep it at 1.0
There was a problem hiding this comment.
I would maybe leave it as latest? Later on we can add in the actual version when we support SvelteKit.
There was a problem hiding this comment.
My worry about leaving it as 1.0 is that we are going to pollute results when we eventually do support SvelteKit and start adding the proper version number.
There was a problem hiding this comment.
fine with me. I was also thinking to convert it to a boolean but I don't want to abuse the module field for that
There was a problem hiding this comment.
Sure, latest is also sounds good to me. As long as the indexer is happy with it.
Our main objective is to find out if SvelteKit is used or not, making the exact version of it lower prio IMO.
Lms24
force-pushed
the
lms-svelte-detect-sveltekit
branch
from
3398cad
to
aa0dd04
Compare
This PR removes the use of the `Element` DOM type from `@sentry/utils`, introduced with #5594. This breaks `@sentry/node` users that use strict TS configs. We have to cast the type to `any` to make sure it works in different environments, but we supply a generic argument so that users can specify what type of `Element` will be returned from `querySelector`.
This PR adds a SvelteKit detection mechanism to our SDK and inserts a SvelteKit entry into
event.modulesif we detect that the frontend in which the SDK is used, was rendered by SvelteKit.To check this, we check for the existence of a div with the id
svelte-announcerthat's automatically injected into the rendered HTML by SvelteKit. It is used to improve accessibility (see sveltejs/kit#307) but we can leverage it as a SvelteKit detection criterion.Btw, we're not the first ones who came up with this approach, as it turns out: https://twitter.com/jhewt/status/1359632380483493889
Additionally, this PR introduces a new utils function,
getDomElementin which we consolidate the usage ofdocument.querySelectorin the SDK. So in addition to using this new function for obtaining the div described above, we now also call it inBrowserTracingto get<meta>tags.Also added some tests to test the new behaviour and the helper functions. We might want to consider writing an integration test for this feature but this first requires a Svelte SDK integration test infrastructure.
ref: #5573