fix(nextjs): preserve directive prologues in turbopack loaders#20103
fix(nextjs): preserve directive prologues in turbopack loaders#20103igz0 wants to merge 4 commits intogetsentry:developfrom
Conversation
sdk-maintainer-bot
bot
added
missing-maintainer-discussion
|
This PR has been automatically closed. The referenced issue does not show a discussion between you and a maintainer. To avoid wasted effort on both sides, please discuss your proposed approach in the issue first and wait for a maintainer to respond before opening a PR. Please review our contributing guidelines for more details. |
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Core
Deps
Other
Bug Fixes 🐛Core
Other
Internal Changes 🔧Core
Deps
Other
🤖 This preview updates automatically when you update the PR. |
andreiborza
removed
violating-contribution-guidelines
JPeer264
left a comment
JPeer264
left a comment
There was a problem hiding this comment.
The changes look good to me overall (I'm a bit far away from Next.js though to verify if the changes are actually valid). I just wonder if all these while loops are the most performant way of doing this.
| let index = 0; | ||
| let lastDirectiveEndIndex: number | undefined; | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
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;There was a problem hiding this comment.
Good point. I updated the loop to use an explicit index < userCode.length exit condition instead of while (true), and added a small EOF test to lock that behavior in.
dd6204f
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.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit dd6204f. Configure here.
s1gr1d
left a comment
s1gr1d
left a comment
There was a problem hiding this comment.
Thank you so much for contributing!
The code is a bit hard to read and reason about. Maybe you can clean it up a bit and add comments when needed.
| continue; | ||
| } | ||
|
|
||
| if (char === '/' && nextChar === '/') { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
| 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; |
There was a problem hiding this comment.
Or even shorter (but maybe less readable):
if (code.startsWith('//', i)) {
i = code.indexOf('\n', i);
if (i === -1) break;
continue;
}There was a problem hiding this comment.
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.
| const statementStartIndex = skipWhitespaceAndComments(userCode, index); | ||
|
|
||
| const nextDirectiveIndex = skipDirective(userCode, statementStartIndex); | ||
| if (nextDirectiveIndex === undefined) { | ||
| return lastDirectiveEndIndex ?? statementStartIndex; | ||
| } | ||
|
|
||
| const statementEndIndex = skipDirectiveTerminator(userCode, nextDirectiveIndex); |
There was a problem hiding this comment.
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;
}There was a problem hiding this comment.
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.
| let lastDirectiveEndIndex: number | undefined; | ||
|
|
||
| while (index < userCode.length) { | ||
| const scanStartIndex = index; |
There was a problem hiding this comment.
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:
scanStartIndexis0andlastDirectiveEndIndexisundefined, so?? scanStartIndex=?? 0 - On subsequent iterations:
lastDirectiveEndIndexis already set, soscanStartIndexis never used
I think you can delete scanStartIndex entirely and write return lastDirectiveEndIndex ?? 0.
| // Only a bare string literal followed by a statement terminator counts as a directive. | ||
| while (statementEndIndex < userCode.length) { |
There was a problem hiding this comment.
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)?
| const scanStartIndex = index; | ||
|
|
||
| // Comments can appear between directive prologue entries, so keep scanning until we reach the next statement. | ||
| while (index < userCode.length) { |
There was a problem hiding this comment.
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.
5 participants
Fixes #20101.
Summary
When
_experimental.turbopackApplicationKeyis enabled, the Next.js Turbopack loaders inject code for metadata/value propagation.The previous implementation relied on a regex that only handled a single directive prologue entry. For modules that start with multiple directives, such as:
the injected code could end up between the directives and break the
"use client"classification.This replaces the regex-based insertion point detection with a small linear scanner that:
Tests
Verification
Using the reproduction from #20101 with the patched
@sentry/nextjstarball:npm run buildcompletes successfully/_not-foundprerendering succeedsTypeError: (0, g.useEffect) is not a functionno longer occurs