Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { LoaderThis } from './types';
import { SKIP_COMMENT_AND_DIRECTIVE_REGEX } from './valueInjectionLoader';
import { findInjectionIndexAfterDirectives } from './valueInjectionLoader';

export type ModuleMetadataInjectionLoaderOptions = {
applicationKey: string;
Expand Down Expand Up @@ -39,7 +39,6 @@ export default function moduleMetadataInjectionLoader(
`e._sentryModuleMetadata[(new e.Error).stack]=Object.assign({},e._sentryModuleMetadata[(new e.Error).stack],${metadata});` +
'}catch(e){}}();';

return userCode.replace(SKIP_COMMENT_AND_DIRECTIVE_REGEX, match => {
return match + injectedCode;
});
const injectionIndex = findInjectionIndexAfterDirectives(userCode);
return `${userCode.slice(0, injectionIndex)}${injectedCode}${userCode.slice(injectionIndex)}`;
}
136 changes: 124 additions & 12 deletions packages/nextjs/src/config/loaders/valueInjectionLoader.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,131 @@
// Rollup doesn't like if we put the directive regex as a literal (?). No idea why.
/* oxlint-disable sdk/no-regexp-constructor */

import type { LoaderThis } from './types';

export type ValueInjectionLoaderOptions = {
values: Record<string, unknown>;
};

// We need to be careful not to inject anything before any `"use strict";`s or "use client"s or really any other directive.
// As an additional complication directives may come after any number of comments.
// This regex is shamelessly stolen from: https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/7f984482c73e4284e8b12a08dfedf23b5a82f0af/packages/bundler-plugin-core/src/index.ts#L535-L539
export const SKIP_COMMENT_AND_DIRECTIVE_REGEX =
// Note: CodeQL complains that this regex potentially has n^2 runtime. This likely won't affect realistic files.
new RegExp('^(?:\\s*|/\\*(?:.|\\r|\\n)*?\\*/|//.*[\\n\\r])*(?:"[^"]*";?|\'[^\']*\';?)?');
// We need to be careful not to inject anything before any `"use strict";`s or "use client"s or really any other
// directives. A small scanner is easier to reason about than the previous regex and avoids regex backtracking concerns.
export function findInjectionIndexAfterDirectives(userCode: string): number {
let index = 0;
let lastDirectiveEndIndex: number | undefined;

while (index < userCode.length) {
const scanStartIndex = index;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This variable is purely to use in fallback returns inside the inner loop (return lastDirectiveEndIndex ?? scanStartIndex). But at that point, scanStartIndex is always equivalent to lastDirectiveEndIndex ?? 0:

  • On the first outer iteration: scanStartIndex is 0 and lastDirectiveEndIndex is undefined, so ?? scanStartIndex = ?? 0
  • On subsequent iterations: lastDirectiveEndIndex is already set, so scanStartIndex is never used

I think you can delete scanStartIndex entirely and write return lastDirectiveEndIndex ?? 0.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good catch. I removed scanStartIndex and simplified those fallback paths to lastDirectiveEndIndex ?? 0 / lastDirectiveEndIndex ?? statementStartIndex, depending on whether we've already reached a concrete statement boundary.

Applied in 215d6bf.


// Comments can appear between directive prologue entries, so keep scanning until we reach the next statement.
while (index < userCode.length) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The outer loop iterates over directives, the inner loop skips whitespace/comments before each one. But the inner loop's work is identical every iteration and it could just be the same flat loop.

A single flat loop handles this just as well and is much easier to follow.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed. I flattened the main scanner so the repeated whitespace/comment skipping is no longer nested inside a second loop. The top-level flow is now: skip trivia, check for a directive string, then scan its terminator.

Applied in 215d6bf.

const char = userCode[index];

if (char && /\s/.test(char)) {
index += 1;
continue;
}

if (userCode.startsWith('//', index)) {
const newlineIndex = userCode.indexOf('\n', index + 2);
index = newlineIndex === -1 ? userCode.length : newlineIndex + 1;
continue;
}

if (userCode.startsWith('/*', index)) {
const commentEndIndex = userCode.indexOf('*/', index + 2);
if (commentEndIndex === -1) {
return lastDirectiveEndIndex ?? scanStartIndex;
}

index = commentEndIndex + 2;
continue;
}

break;
}

const statementStartIndex = index;
const quote = userCode[statementStartIndex];
if (quote !== '"' && quote !== "'") {
return lastDirectiveEndIndex ?? statementStartIndex;
}

const stringEndIndex = findStringLiteralEnd(userCode, statementStartIndex);
if (stringEndIndex === undefined) {
return lastDirectiveEndIndex ?? statementStartIndex;
}

let statementEndIndex = stringEndIndex;

// Only a bare string literal followed by a statement terminator counts as a directive.
while (statementEndIndex < userCode.length) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know I made a comment in the last review questioning all the helper functions, but that was more about "do we actually need to check the end of the statement or can we do this in a shorter way?" - but the overall goal is better readability.

I understand now, that you specifically want to check if the directive is properly terminated. This loop is deeply nested and could be extracted in its own function (maybe scanDirectiveTerminator(code, i) or something like this)?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Makes sense. I pulled the post-string directive validation into a small findDirectiveTerminator helper so the main scan no longer has that deeply nested loop inline, while still preserving the stricter "properly terminated directive" check.

Applied in 215d6bf.

const char = userCode[statementEndIndex];

if (char === ';') {
statementEndIndex += 1;
break;
}

if (char === '\n' || char === '\r' || char === '}') {
break;
}

if (char && /\s/.test(char)) {
statementEndIndex += 1;
continue;
}

if (userCode.startsWith('//', statementEndIndex)) {
break;
}

if (userCode.startsWith('/*', statementEndIndex)) {
const commentEndIndex = userCode.indexOf('*/', statementEndIndex + 2);
if (commentEndIndex === -1) {
return lastDirectiveEndIndex ?? statementStartIndex;
}

const comment = userCode.slice(statementEndIndex + 2, commentEndIndex);
if (comment.includes('\n') || comment.includes('\r')) {
break;
}

statementEndIndex = commentEndIndex + 2;
continue;
}

return lastDirectiveEndIndex ?? statementStartIndex;
}

index = statementEndIndex;
lastDirectiveEndIndex = statementEndIndex;
}

return lastDirectiveEndIndex ?? index;
}

function findStringLiteralEnd(userCode: string, startIndex: number): number | undefined {
const quote = userCode[startIndex];
let index = startIndex + 1;

while (index < userCode.length) {
const char = userCode[index];

if (char === '\\') {
index += 2;
continue;
}

if (char === quote) {
return index + 1;
}

if (char === '\n' || char === '\r') {
return undefined;
}

index += 1;
}

return undefined;
}

/**
* Set values on the global/window object at the start of a module.
Expand All @@ -36,7 +149,6 @@ export default function valueInjectionLoader(this: LoaderThis<ValueInjectionLoad
.map(([key, value]) => `globalThis["${key}"] = ${JSON.stringify(value)};`)
.join('');

return userCode.replace(SKIP_COMMENT_AND_DIRECTIVE_REGEX, match => {
return match + injectedCode;
});
const injectionIndex = findInjectionIndexAfterDirectives(userCode);
return `${userCode.slice(0, injectionIndex)}${injectedCode}${userCode.slice(injectionIndex)}`;
}
26 changes: 26 additions & 0 deletions packages/nextjs/test/config/moduleMetadataInjectionLoader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,4 +131,30 @@ describe('moduleMetadataInjectionLoader', () => {

expect(result).toContain('"_sentryBundlerPluginAppKey:test-key-123":true');
});

it('should inject after multiple directives', () => {
const loaderThis = createLoaderThis('my-app');
const userCode = '"use strict";\n"use client";\nimport React from \'react\';';

const result = moduleMetadataInjectionLoader.call(loaderThis, userCode);

const metadataIndex = result.indexOf('_sentryModuleMetadata');
const clientDirectiveIndex = result.indexOf('"use client"');
const importIndex = result.indexOf("import React from 'react';");

expect(metadataIndex).toBeGreaterThan(clientDirectiveIndex);
expect(metadataIndex).toBeLessThan(importIndex);
});

it('should inject after comments between multiple directives', () => {
const loaderThis = createLoaderThis('my-app');
const userCode = '"use strict";\n/* keep */\n"use client";\nimport React from \'react\';';

const result = moduleMetadataInjectionLoader.call(loaderThis, userCode);

const metadataIndex = result.indexOf('_sentryModuleMetadata');
const clientDirectiveIndex = result.indexOf('"use client"');

expect(metadataIndex).toBeGreaterThan(clientDirectiveIndex);
});
});
85 changes: 84 additions & 1 deletion packages/nextjs/test/config/valueInjectionLoader.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { describe, expect, it } from 'vitest';
import type { LoaderThis } from '../../src/config/loaders/types';
import type { ValueInjectionLoaderOptions } from '../../src/config/loaders/valueInjectionLoader';
import valueInjectionLoader from '../../src/config/loaders/valueInjectionLoader';
import valueInjectionLoader, { findInjectionIndexAfterDirectives } from '../../src/config/loaders/valueInjectionLoader';

const defaultLoaderThis = {
addDependency: () => undefined,
Expand Down Expand Up @@ -149,4 +149,87 @@ describe.each([[clientConfigLoaderThis], [instrumentationLoaderThis]])('valueInj
expect(result).toMatchSnapshot();
expect(result).toMatch(';globalThis["foo"] = "bar";');
});

it('should correctly insert values after multiple directives', () => {
const userCode = `
"use strict";
"use client";
import * as Sentry from '@sentry/nextjs';
Sentry.init();
`;

const result = valueInjectionLoader.call(loaderThis, userCode);

const injectionIndex = result.indexOf(';globalThis["foo"] = "bar";');
const clientDirectiveIndex = result.indexOf('"use client"');
const importIndex = result.indexOf("import * as Sentry from '@sentry/nextjs';");

expect(injectionIndex).toBeGreaterThan(clientDirectiveIndex);
expect(injectionIndex).toBeLessThan(importIndex);
});

it('should correctly insert values after comments between multiple directives', () => {
const userCode = `
"use strict";
/* keep */
"use client";
import * as Sentry from '@sentry/nextjs';
Sentry.init();
`;

const result = valueInjectionLoader.call(loaderThis, userCode);

const injectionIndex = result.indexOf(';globalThis["foo"] = "bar";');
const clientDirectiveIndex = result.indexOf('"use client"');

expect(injectionIndex).toBeGreaterThan(clientDirectiveIndex);
});

it('should correctly insert values after semicolon-free directives', () => {
const userCode = `
"use strict"
"use client"
import * as Sentry from '@sentry/nextjs';
Sentry.init();
`;

const result = valueInjectionLoader.call(loaderThis, userCode);

const injectionIndex = result.indexOf(';globalThis["foo"] = "bar";');
const clientDirectiveIndex = result.indexOf('"use client"');

expect(injectionIndex).toBeGreaterThan(clientDirectiveIndex);
});
});

describe('findInjectionIndexAfterDirectives', () => {
it('returns the position immediately after the last directive', () => {
const userCode = '"use strict";\n"use client";\nimport React from \'react\';';

expect(userCode.slice(findInjectionIndexAfterDirectives(userCode))).toBe("\nimport React from 'react';");
});

it('returns the end of the input when the last directive reaches EOF', () => {
const userCode = '"use strict";\n"use client";';

expect(findInjectionIndexAfterDirectives(userCode)).toBe(userCode.length);
});

it('does not skip a string literal that is not a directive', () => {
const userCode = '"use client" + suffix;';

expect(findInjectionIndexAfterDirectives(userCode)).toBe(0);
});

it('does not treat an escaped quote at EOF as a closed directive', () => {
const userCode = '"use client\\"';

expect(findInjectionIndexAfterDirectives(userCode)).toBe(0);
});

it('treats a block comment without a line break as part of the same statement', () => {
const userCode = '"use client" /* comment */ + suffix;';

expect(findInjectionIndexAfterDirectives(userCode)).toBe(0);
});
});
Loading