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(sveltekit): Ensure target file exists before applying auto instrumentation #8881

Merged
merged 2 commits into from Aug 29, 2023

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Aug 28, 2023

In our auto instrumentation Vite plugin for SvelteKit, we read +(page|layout)(.server).(js|ts) files' code to determine if we should add our wrapper to the file or not. We previously didn't check for a file id's existence before reading the file if the id matched that certain pattern, wrongly assuming that these ids would always map to actually existing files.

It seems like Vite plugins such as Houdini's plugin add file ids to the build for files that actually don't exist (#8846, #8854) . When our plugin's load hook was called for such an id, it then tried to access and read the file which caused a build error.

This patch now adds a file existence guard to ensure we simply no-op for these files.

(Wild guess: Houdini adds load functions for all (some?) routes so that it can do its graph QL magic under the hood. Meaning, if we no-op at build time, these functions might not be instrumented with Sentry spans. It's probably not the end of the world as errors should still be caught by the handleError hook. But let's see. I already cc'd the Houdini maintainers in #8846 to let them know of this).

closes #8846
closes #8854

@Lms24 Lms24 marked this pull request as ready for review August 28, 2023 14:20
@Lms24 Lms24 requested review from a team, mydea and lforst and removed request for a team August 28, 2023 14:20
@Lms24 Lms24 changed the title fix(sveltekit): Ensure file exists before applying auto instrumentation fix(sveltekit): Ensure target file exists before applying auto instrumentation Aug 28, 2023
Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

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

ah yes virtual modules

@Lms24 Lms24 merged commit e3dda4c into develop Aug 29, 2023
40 checks passed
@Lms24 Lms24 deleted the lms/sveltekit-fix-autoinstrument-file-not-found branch August 29, 2023 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants