-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
[Flight] Allow parens in filenames when parsing stackframes #30396
Changes from all commits
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 |
---|---|---|
|
@@ -10,6 +10,8 @@ | |
|
||
'use strict'; | ||
|
||
const path = require('path'); | ||
|
||
if (typeof Blob === 'undefined') { | ||
global.Blob = require('buffer').Blob; | ||
} | ||
|
@@ -41,6 +43,26 @@ function formatV8Stack(stack) { | |
return v8StyleStack; | ||
} | ||
|
||
const repoRoot = path.resolve(__dirname, '../../../../'); | ||
function normalizeReactCodeLocInfo(str) { | ||
const repoRootForRegexp = repoRoot.replace(/\//g, '\\/'); | ||
const repoFileLocMatch = new RegExp(`${repoRootForRegexp}.+?:\\d+:\\d+`, 'g'); | ||
return str && str.replace(repoFileLocMatch, '**'); | ||
} | ||
|
||
// If we just use the original Error prototype, Jest will only display the error message if assertions fail. | ||
// But we usually want to also assert on our expando properties or even the stack. | ||
// By hiding the fact from Jest that this is an error, it will show all enumerable properties on mismatch. | ||
|
||
function getErrorForJestMatcher(error) { | ||
return { | ||
...error, | ||
// non-enumerable properties that are still relevant for testing | ||
message: error.message, | ||
stack: normalizeReactCodeLocInfo(error.stack), | ||
}; | ||
} | ||
|
||
function normalizeComponentInfo(debugInfo) { | ||
if (Array.isArray(debugInfo.stack)) { | ||
const {debugTask, debugStack, ...copy} = debugInfo; | ||
|
@@ -1177,6 +1199,135 @@ describe('ReactFlight', () => { | |
}); | ||
}); | ||
|
||
it('should handle exotic stack frames', async () => { | ||
function ServerComponent() { | ||
const error = new Error('This is an error'); | ||
const originalStackLines = error.stack.split('\n'); | ||
// Fake a stack | ||
error.stack = [ | ||
originalStackLines[0], | ||
// original | ||
// ' at ServerComponentError (file://~/react/packages/react-client/src/__tests__/ReactFlight-test.js:1166:19)', | ||
// nested eval (https://github.com/ChromeDevTools/devtools-frontend/blob/831be28facb4e85de5ee8c1acc4d98dfeda7a73b/test/unittests/front_end/panels/console/ErrorStackParser_test.ts#L198) | ||
' at eval (eval at testFunction (inspected-page.html:29:11), <anonymous>:1:10)', | ||
// parens may be added by Webpack when bundle layers are used. They're also valid in directory names. | ||
' at ServerComponentError (file://~/(some)(really)(exotic-directory)/ReactFlight-test.js:1166:19)', | ||
// anon function (https://github.com/ChromeDevTools/devtools-frontend/blob/831be28facb4e85de5ee8c1acc4d98dfeda7a73b/test/unittests/front_end/panels/console/ErrorStackParser_test.ts#L115C9-L115C35) | ||
' at file:///testing.js:42:3', | ||
// async anon function (https://github.com/ChromeDevTools/devtools-frontend/blob/831be28facb4e85de5ee8c1acc4d98dfeda7a73b/test/unittests/front_end/panels/console/ErrorStackParser_test.ts#L130C9-L130C41) | ||
' at async file:///testing.js:42:3', | ||
...originalStackLines.slice(2), | ||
].join('\n'); | ||
throw error; | ||
} | ||
|
||
const findSourceMapURL = jest.fn(() => null); | ||
const errors = []; | ||
class MyErrorBoundary extends React.Component { | ||
state = {error: null}; | ||
static getDerivedStateFromError(error) { | ||
return {error}; | ||
} | ||
componentDidCatch(error, componentInfo) { | ||
errors.push(error); | ||
} | ||
render() { | ||
if (this.state.error) { | ||
return null; | ||
} | ||
return this.props.children; | ||
} | ||
} | ||
const ClientErrorBoundary = clientReference(MyErrorBoundary); | ||
|
||
function App() { | ||
return ( | ||
<ClientErrorBoundary> | ||
<ServerComponent /> | ||
</ClientErrorBoundary> | ||
); | ||
} | ||
|
||
const transport = ReactNoopFlightServer.render(<App />, { | ||
onError(x) { | ||
if (__DEV__) { | ||
return 'a dev digest'; | ||
} | ||
if (x instanceof Error) { | ||
return `digest("${x.message}")`; | ||
} else if (Array.isArray(x)) { | ||
return `digest([])`; | ||
} else if (typeof x === 'object' && x !== null) { | ||
return `digest({})`; | ||
} | ||
return `digest(${String(x)})`; | ||
}, | ||
}); | ||
|
||
await act(() => { | ||
startTransition(() => { | ||
ReactNoop.render( | ||
ReactNoopFlightClient.read(transport, {findSourceMapURL}), | ||
); | ||
}); | ||
}); | ||
|
||
if (__DEV__) { | ||
expect({ | ||
errors: errors.map(getErrorForJestMatcher), | ||
findSourceMapURLCalls: findSourceMapURL.mock.calls, | ||
}).toEqual({ | ||
Comment on lines
+1276
to
+1279
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. 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 There's obviously a limit depending on the kind of assertion. But this is not one of those in my opinion. |
||
errors: [ | ||
{ | ||
message: 'This is an error', | ||
stack: gate(flags => flags.enableOwnerStacks) | ||
? expect.stringContaining( | ||
'Error: This is an error\n' + | ||
' at eval (eval at testFunction (eval at createFakeFunction (**), <anonymous>:1:35)\n' + | ||
' at ServerComponentError (file://~/(some)(really)(exotic-directory)/ReactFlight-test.js:1166:19)\n' + | ||
' at (anonymous) (file:///testing.js:42:3)\n' + | ||
' at (anonymous) (file:///testing.js:42:3)\n', | ||
) | ||
: expect.stringContaining( | ||
'Error: This is an error\n' + | ||
' at eval (eval at testFunction (inspected-page.html:29:11), <anonymous>:1:10)\n' + | ||
' at ServerComponentError (file://~/(some)(really)(exotic-directory)/ReactFlight-test.js:1166:19)\n' + | ||
' at file:///testing.js:42:3\n' + | ||
' at file:///testing.js:42:3', | ||
), | ||
digest: 'a dev digest', | ||
environmentName: 'Server', | ||
}, | ||
], | ||
findSourceMapURLCalls: gate(flags => flags.enableOwnerStacks) | ||
? [ | ||
[__filename], | ||
[__filename], | ||
Comment on lines
+1304
to
+1305
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. Interesting that we call 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. 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. |
||
// TODO: What should we request here? The outer (<anonymous>) or the inner (inspected-page.html)? | ||
['inspected-page.html:29:11), <anonymous>'], | ||
['file://~/(some)(really)(exotic-directory)/ReactFlight-test.js'], | ||
['file:///testing.js'], | ||
[__filename], | ||
] | ||
: [], | ||
}); | ||
} else { | ||
expect(errors.map(getErrorForJestMatcher)).toEqual([ | ||
{ | ||
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.', | ||
Comment on lines
+1317
to
+1324
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. 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. |
||
digest: 'digest("This is an error")', | ||
}, | ||
]); | ||
} | ||
}); | ||
|
||
it('should include server components in warning stacks', async () => { | ||
function Component() { | ||
// Trigger key warning | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using
expect
in React lifecycles has a couple of problems: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.