-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(nextjs): preserve directive prologues in turbopack loaders #20103
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
Changes from 1 commit
a648434
dd6204f
81516b3
c1d0518
215d6bf
75b7f19
60f6066
547b6d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,18 +1,149 @@ | ||||||||||||||||||
| // 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 (true) { | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. l: This while loop could potentially run forever. Would it make sense to have some return at a specific index number? E.g. if (index >= 10) return lastDirectiveEndIndex;
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I updated the loop to use an explicit I didn't add a fixed index cap, since that could incorrectly stop on valid inputs with longer comments or multiple directives. The scanner still does a single forward pass over the input, so the runtime remains linear in the module length. |
||||||||||||||||||
| const statementStartIndex = skipWhitespaceAndComments(userCode, index); | ||||||||||||||||||
|
|
||||||||||||||||||
| const nextDirectiveIndex = skipDirective(userCode, statementStartIndex); | ||||||||||||||||||
| if (nextDirectiveIndex === undefined) { | ||||||||||||||||||
| return lastDirectiveEndIndex ?? statementStartIndex; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| const statementEndIndex = skipDirectiveTerminator(userCode, nextDirectiveIndex); | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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;
}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| 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 === '/') { | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Then you also don't need the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, updated this to use Applied in c1d0518. |
||||||||||||||||||
| index += 2; | ||||||||||||||||||
| while (index < userCode.length && userCode[index] !== '\n' && userCode[index] !== '\r') { | ||||||||||||||||||
| index += 1; | ||||||||||||||||||
| } | ||||||||||||||||||
| continue; | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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;
}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, I simplified the line-comment handling here to use 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; | ||||||||||||||||||
| } | ||||||||||||||||||
|
cursor[bot] marked this conversation as resolved.
Outdated
|
||||||||||||||||||
|
|
||||||||||||||||||
| 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. | ||||||||||||||||||
|
|
@@ -36,7 +167,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)}`; | ||||||||||||||||||
| } | ||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.