Skip to content

Commit

Permalink
fix(sveltekit): Ensure target file exists before applying auto instru…
Browse files Browse the repository at this point in the history
…mentation (#8881)

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.
  • Loading branch information
Lms24 committed Aug 29, 2023
1 parent ff0da3e commit e3dda4c
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 6 deletions.
18 changes: 15 additions & 3 deletions packages/sveltekit/src/vite/autoInstrument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ type AutoInstrumentPluginOptions = AutoInstrumentSelection & {
* @returns the plugin
*/
export function makeAutoInstrumentationPlugin(options: AutoInstrumentPluginOptions): Plugin {
const { load: shouldWrapLoad, serverLoad: shouldWrapServerLoad, debug } = options;
const { load: wrapLoadEnabled, serverLoad: wrapServerLoadEnabled, debug } = options;

return {
name: 'sentry-auto-instrumentation',
Expand All @@ -49,7 +49,7 @@ export function makeAutoInstrumentationPlugin(options: AutoInstrumentPluginOptio

async load(id) {
const applyUniversalLoadWrapper =
shouldWrapLoad &&
wrapLoadEnabled &&
/^\+(page|layout)\.(js|ts|mjs|mts)$/.test(path.basename(id)) &&
(await canWrapLoad(id, debug));

Expand All @@ -60,7 +60,7 @@ export function makeAutoInstrumentationPlugin(options: AutoInstrumentPluginOptio
}

const applyServerLoadWrapper =
shouldWrapServerLoad &&
wrapServerLoadEnabled &&
/^\+(page|layout)\.server\.(js|ts|mjs|mts)$/.test(path.basename(id)) &&
(await canWrapLoad(id, debug));

Expand Down Expand Up @@ -90,7 +90,19 @@ export function makeAutoInstrumentationPlugin(options: AutoInstrumentPluginOptio
* @returns `true` if we can wrap the given file, `false` otherwise
*/
export async function canWrapLoad(id: string, debug: boolean): Promise<boolean> {
// Some 3rd party plugins add ids to the build that actually don't exist.
// We need to check for that here, otherwise users get get a build errors.
if (!fs.existsSync(id)) {
debug &&
// eslint-disable-next-line no-console
console.log(
`Skipping wrapping ${id} because it doesn't exist. A 3rd party plugin might have added this as a virtual file to the build`,
);
return false;
}

const code = (await fs.promises.readFile(id, 'utf8')).toString();

const mod = parseModule(code);

const program = mod.$ast.type === 'Program' && mod.$ast;
Expand Down
17 changes: 14 additions & 3 deletions packages/sveltekit/test/vite/autoInstrument.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ vi.mock('fs', async () => {
return fileContent || DEFAULT_CONTENT;
}),
},
existsSync: vi.fn().mockImplementation(id => {
if (id === '+page.virtual.ts') {
return false;
}
return true;
}),
};
});

Expand Down Expand Up @@ -198,15 +204,20 @@ describe('canWrapLoad', () => {
'export const loadNotLoad = () => {}; export const prerender = true;',
'export function aload(){}; export const prerender = true;',
'export function loader(){}; export const prerender = true;',
'let loademe = false; export {loadme}',
'let loadme = false; export {loadme}',
'const a = {load: true}; export {a}',
'if (s === "load") {}',
'const a = load ? load : false',
'// const load = () => {}',
'/* export const load = () => {} */ export const prerender = true;',
'/* export const notLoad = () => { const a = getSomething() as load; } */ export const prerender = true;',
])('returns `false` if no load declaration exists', async (_, code) => {
])('returns `false` if no load declaration exists', async code => {
fileContent = code;
expect(await canWrapLoad('+page.ts', false)).toEqual(true);
expect(await canWrapLoad('+page.ts', false)).toEqual(false);
});

it("returns `false` if the passed file id doesn't exist", async () => {
fileContent = DEFAULT_CONTENT;
expect(await canWrapLoad('+page.virtual.ts', false)).toEqual(false);
});
});

0 comments on commit e3dda4c

Please sign in to comment.