Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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)}`;
}
156 changes: 144 additions & 12 deletions packages/nextjs/src/config/loaders/valueInjectionLoader.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,151 @@
// 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 statementStartIndex = skipWhitespaceAndComments(userCode, index);

const nextDirectiveIndex = skipDirective(userCode, statementStartIndex);
if (nextDirectiveIndex === undefined) {
return lastDirectiveEndIndex ?? statementStartIndex;
}

const statementEndIndex = skipDirectiveTerminator(userCode, nextDirectiveIndex);
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 don't think we need three functions here (as all of them are doing similar things). The only thing we want to find is the correct place for injection.

What do you think of something like this? This would just return the number after the last directive. But maybe I am missing something here.

export function findInjectionIndex(code: string): number {
  let i = 0;
  let afterLastDirective = 0;

  while (i < code.length) {
    const char = code[i];

    // Skip whitespace
    if (char && /\s/.test(char)) {
      i++;
      continue;
    }

    // Skip line comments
    if (code.startsWith('//', i)) {
      const newline = code.indexOf('\n', i + 2);
      i = newline === -1 ? code.length : newline + 1;
      continue;
    }

    // Skip block comments
    if (code.startsWith('/*', i)) {
      const end = code.indexOf('*/', i + 2);
      i = end === -1 ? code.length : end + 2;
      continue;
    }

    // Match a directive (a quoted string at statement position)
    if (char === '"' || char === "'") {
      const close = code.indexOf(char, i + 1);
      if (close !== -1) {
        let end = close + 1;
        if (code[end] === ';') end++;
        afterLastDirective = end;
        i = end;
        continue;
      }
    }

    // Hit something anything else that's not a comment, ws, or directive: stop scanning
    break;
  }

  return afterLastDirective;
}

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.

Thanks, this direction makes sense.

I refactored the directive scan so the logic now lives in a single scanner flow instead of being split across three helpers. I kept only a small string-literal helper so we still handle escaped/unterminated quote cases correctly.

Applied in c1d0518.

if (statementEndIndex === undefined) {
return lastDirectiveEndIndex ?? statementStartIndex;
}

index = statementEndIndex;
lastDirectiveEndIndex = statementEndIndex;
}

return lastDirectiveEndIndex ?? index;
}

function skipWhitespaceAndComments(userCode: string, startIndex: number): number {
let index = startIndex;

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

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

if (char === '/' && nextChar === '/') {
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.

To reduce bundle size and improve readability of the code, you could use userCode.startsWith('//', index) (also the same with /*).

Then you also don't need the nextChar variable.

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 point, updated this to use userCode.startsWith('//', index) / userCode.startsWith('/*', index) and dropped the extra nextChar handling.

Applied in c1d0518.

index += 2;
while (index < userCode.length && userCode[index] !== '\n' && userCode[index] !== '\r') {
index += 1;
}
continue;
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 could also be written in a shorter, more readable way. There is no need for a while loop as we can get the new index from indexOf. And we also don't need to check for /r as Windows-style line endings (\r\n) are already handled with \n.

Suggested change
index += 2;
while (index < userCode.length && userCode[index] !== '\n' && userCode[index] !== '\r') {
index += 1;
}
continue;
const newline = code.indexOf('\n', i + 2);
i = newline === -1 ? code.length : newline + 1;
continue;

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.

Or even shorter (but maybe less readable):

    if (code.startsWith('//', i)) {
      i = code.indexOf('\n', i);
      if (i === -1) break;
      continue;
    }

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.

Yep, I simplified the line-comment handling here to use indexOf('\n', index + 2), which makes this path shorter and easier to read.

Applied in c1d0518.

}

if (char === '/' && nextChar === '*') {
const commentEndIndex = userCode.indexOf('*/', index + 2);
if (commentEndIndex === -1) {
return startIndex;
}

index = commentEndIndex + 2;
continue;
}

return index;
}

return index;
}

function skipDirective(userCode: string, startIndex: number): number | undefined {
const quote = userCode[startIndex];

if (quote !== '"' && quote !== "'") {
return undefined;
}

let index = startIndex + 1;

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

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

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

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

index += 1;
}

if (index > userCode.length || userCode[index - 1] !== quote) {
return undefined;
}

return index;
}

function skipDirectiveTerminator(userCode: string, startIndex: number): number | undefined {
let index = startIndex;

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

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

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

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

if (char === '/' && nextChar === '/') {
return index;
}

if (char === '/' && nextChar === '*') {
const commentEndIndex = userCode.indexOf('*/', index + 2);
if (commentEndIndex === -1) {
return undefined;
}

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

index = commentEndIndex + 2;
continue;
}

return undefined;
}

return index;
}

/**
* Set values on the global/window object at the start of a module.
Expand All @@ -36,7 +169,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);
});
});
79 changes: 78 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,81 @@ 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('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);
});
});