[Flight] Allow parens in filenames when parsing stackframes #30396
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: e902c45...3f6be93 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
| return {error}; | ||
| } | ||
| componentDidCatch(error, componentInfo) { | ||
| errors.push(error); |
There was a problem hiding this comment.
using expect in React lifecycles has a couple of problems:
- if they fail, the error reporting is unreadable since we now have AggregateError and also the frame is pointing all throughout React internals
- If we'd stop calling this particular lifecycle, tests would pass.
As far as I can tell, this "persist this intermediate value and then assert after render" is widely used so this isn't a new precedence.
| expect({ | ||
| errors: errors.map(getErrorForJestMatcher), | ||
| findSourceMapURLCalls: findSourceMapURL.mock.calls, | ||
| }).toEqual({ |
There was a problem hiding this comment.
Single assertions are why more helpful over multiple assertions if one of them fails. A clue for the failure reason may be in later assertions and there may not be an optimal ordering. Packing everything in a single expect gives you the full context on mismatch.
There's obviously a limit depending on the kind of assertion. But this is not one of those in my opinion.
| message: | ||
| 'An error occurred in the Server Components render. The specific message is omitted in production' + | ||
| ' builds to avoid leaking sensitive details. A digest property is included on this error instance which' + | ||
| ' may provide additional details about the nature of the error.', | ||
| stack: | ||
| 'Error: An error occurred in the Server Components render. The specific message is omitted in production' + | ||
| ' builds to avoid leaking sensitive details. A digest property is included on this error instance which' + | ||
| ' may provide additional details about the nature of the error.', |
There was a problem hiding this comment.
If we agree that the pattern in this test is better, I'll rewrite the other tests and then it may be better to move this into a variable. Though we usually prefer having explicit message in tests so maybe not.
| message: 'This is an error', | ||
| stack: expect.stringContaining( | ||
| 'Error: This is an error\n' + | ||
| ' at eval (eval at testFunction (eval at createFakeFunction (**), <anonymous>:1:35)\n' + |
There was a problem hiding this comment.
Not sure how createFakeFunction ends up here especially because the frame now has non-matching parens.
eps1lon
force-pushed
the
parse-stackframes
branch
from
dc8580f
to
bccee1b
Compare
eps1lon
force-pushed
the
parse-stackframes
branch
from
bccee1b
to
2bbfc8a
Compare
eps1lon
force-pushed
the
parse-stackframes
branch
from
2bbfc8a
to
f619f24
Compare
| [__filename], | ||
| [__filename], |
There was a problem hiding this comment.
Interesting that we call findSourceMapURL multiple times for the same file.
There was a problem hiding this comment.
Only if it is different line/column numbers. In theory we could even pass the those as arguments in case it's useful info.
It might be nice for the implementation to batch multiple line/columns into one eval too. Right now it's one eval per location - not one per file.
Summary
Based on #30395. PR has separate commits for current behavior and proposed behavior.
filenames can have parens so we need to account for that when parsing stack frames. We found that bug when trying out the latest React Canary in Next.js and stack frames from Server Components disappeared when the error was handled by a Client error boundary. Webpack adds parens for bundling layers and parens are used in Next.js to indicate route groups (and are just generally valid characters on common operating systems).
How did you test this change?
test/development/acceptance-app/rsc-runtime-errors.test.ts: "Error overlay - RSC runtime errors > should not show the bundle layer info in the file trace"): [not for merge] Allow parens in Server stack frames vercel/next.js#68120findSourceMapURLCalls