-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
feat[devtools]: symbolicate source for inspected element #28471
feat[devtools]: symbolicate source for inspected element #28471
Conversation
Comparing: 61bd004...2a1097f Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
4f61d6f
to
74b254d
Compare
74b254d
to
439594b
Compare
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.
This all seems broadly reasonable. Added some thoughts. In general, making source map logic reliable is a matter of testing, testing and more testing, so I would encourage you to beef up the automated test coverage here as necessary.
); | ||
|
||
// This is a hacky way to make it work for Next, since their URLs are not normalized | ||
const normalizedReferenceURL = url.replace('/./', '/'); |
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.
Is there some better way to normalise URLs (e.g. with a spec-compliant URL parser?) What does Chromium actually use for this normalisation?
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.
There is https://www.npmjs.com/package/normalize-url, but it doesn't support custom protocols, like webpack
or webpack-internal
.
For now I think its just worth moving to a function -
react/packages/react-devtools-shared/src/utils.js
Lines 959 to 962 in eb9289f
// This is a hacky one to just support this exact case. | |
export function normalizeUrl(url: string): string { | |
return url.replace('/./', '/'); | |
} |
Don't really want RDT to support every possible bundler behaviour and perform this URL management. If we end up having such cases, which would require some support from us explicitly, then we can update the normalizeURL
implementation.
|
||
// This is a hacky way to make it work for Next, since their URLs are not normalized | ||
const normalizedReferenceURL = url.replace('/./', '/'); | ||
const resource = resources.find(r => r.url === normalizedReferenceURL); |
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.
Should we subscribe to onResourceAdded
and maintain a Map
of resources instead of doing an O(n) find
for every URL?
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.
We can't because this event is emitted once resource is added: if main frame resources are loaded and user then opens browser DevTools, the listener won't be called with main frame resources.
If this gets slow for larger apps, I can work on some caching and normalization (to get resource in O(1)), probably
@@ -297,7 +297,7 @@ export function parseSourceFromComponentStack( | |||
const frames = componentStack.split('\n'); | |||
// eslint-disable-next-line no-for-of-loops/no-for-of-loops | |||
for (const frame of frames) { | |||
const openingBracketIndex = frame.lastIndexOf('('); | |||
const openingBracketIndex = frame.indexOf('('); |
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.
Are there unit tests for this logic? Would love to see the concrete cases affected by this.
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.
Thanks for flagging, I will move it to #28351 and add unit tests
|
||
// This function is based on describeComponentFrame() in packages/shared/ReactComponentStackFrame | ||
function formatSourceForDisplay(sourceURL: string, line: number) { | ||
// Note: this RegExp doesn't work well with URLs from Metro, |
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.
What would it take to use https://www.npmjs.com/package/jsc-safe-url here?
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.
Didn't know we have this, will use it, thanks
eb9289f
to
fb8cdeb
Compare
… stacks (#28351) `_debugSource` was removed in #28265. This PR migrates DevTools to define `source` for Fiber based on component stacks. This will be done lazily for inspected elements, once user clicks on the element in the tree. `DevToolsComponentStackFrame.js` was just copy-pasted from the implementation in `ReactComponentStackFrame`. Symbolication part is done in #28471 and stacked on this commit.
fb8cdeb
to
2a1097f
Compare
* feat[devtools]: symbolicate source for inspected element ([hoxyq](https://github.com/hoxyq) in [#28471](#28471)) * refactor[devtools]: lazily define source for fiber based on component stacks ([hoxyq](https://github.com/hoxyq) in [#28351](#28351)) * fix[devtools/tree/element]: onClick -> onMouseDown to handle first click correctly ([hoxyq](https://github.com/hoxyq) in [#28486](#28486)) * [DOM] disable legacy mode behind flag ([gnoff](https://github.com/gnoff) in [#28468](#28468)) * Fix Broken Links In Documentation ([justindhillon](https://github.com/justindhillon) in [#28321](#28321)) * Update /link URLs to react.dev ([rickhanlonii](https://github.com/rickhanlonii) in [#28477](#28477)) * [tests] add support for @GATE pragma ([gnoff](https://github.com/gnoff) in [#28479](#28479)) * Devtools: Unwrap Promise in useFormState ([eps1lon](https://github.com/eps1lon) in [#28319](#28319)) * Add support for rendering BigInt ([eps1lon](https://github.com/eps1lon) in [#24580](#24580)) * Include server component names in the componentStack in DEV ([sebmarkbage](https://github.com/sebmarkbage) in [#28415](#28415))
DevTools e2e tests started to fail after landing #28471: - https://app.circleci.com/pipelines/github/facebook/react/50984/workflows/a7be25ed-9547-40e9-87bd-b14d9d2e87da/jobs/798270 - https://app.circleci.com/pipelines/github/facebook/react/50984/workflows/a7be25ed-9547-40e9-87bd-b14d9d2e87da/jobs/798275 - https://app.circleci.com/pipelines/github/facebook/react/50984/workflows/a7be25ed-9547-40e9-87bd-b14d9d2e87da/jobs/798271 - https://app.circleci.com/pipelines/github/facebook/react/50984/workflows/a7be25ed-9547-40e9-87bd-b14d9d2e87da/jobs/798274 - https://app.circleci.com/pipelines/github/facebook/react/50984/workflows/a7be25ed-9547-40e9-87bd-b14d9d2e87da/jobs/798269 There are 2 reasons for that: 1. Versions 16.0 and 16.5 use legacy renderer, which doesn't support source inspection by design: https://github.com/facebook/react/blob/850fac4915864a487e7cb9ecae8a75dbac144174/packages/react-devtools-shared/src/backend/legacy/renderer.js#L831 The corresponding e2e test is now gated for versions >=16.8 2. For other versions (>=16.8), the source is actually `e2e-app-regression.js`, because these regression tests open a different page (not the one we open for tests against React from source) https://github.com/facebook/react/blob/850fac4915864a487e7cb9ecae8a75dbac144174/packages/react-devtools-inline/playwright.config.js#L15-L17
- facebook/react#28596 - facebook/react#28625 - facebook/react#28616 - facebook/react#28491 - facebook/react#28583 - facebook/react#28427 - facebook/react#28613 - facebook/react#28599 - facebook/react#28611 - facebook/react#28610 - facebook/react#28606 - facebook/react#28598 - facebook/react#28549 - facebook/react#28557 - facebook/react#28467 - facebook/react#28591 - facebook/react#28459 - facebook/react#28590 - facebook/react#28564 - facebook/react#28582 - facebook/react#28579 - facebook/react#28578 - facebook/react#28521 - facebook/react#28550 - facebook/react#28576 - facebook/react#28577 - facebook/react#28571 - facebook/react#28572 - facebook/react#28560 - facebook/react#28569 - facebook/react#28573 - facebook/react#28546 - facebook/react#28568 - facebook/react#28562 - facebook/react#28566 - facebook/react#28565 - facebook/react#28559 - facebook/react#28508 - facebook/react#20432 - facebook/react#28555 - facebook/react#24730 - facebook/react#28472 - facebook/react#27991 - facebook/react#28514 - facebook/react#28548 - facebook/react#28526 - facebook/react#28515 - facebook/react#28533 - facebook/react#28532 - facebook/react#28531 - facebook/react#28407 - facebook/react#28522 - facebook/react#28538 - facebook/react#28509 - facebook/react#28534 - facebook/react#28527 - facebook/react#28528 - facebook/react#28519 - facebook/react#28411 - facebook/react#28520 - facebook/react#28518 - facebook/react#28493 - facebook/react#28504 - facebook/react#28499 - facebook/react#28501 - facebook/react#28496 - facebook/react#28471 - facebook/react#28351 - facebook/react#28486 - facebook/react#28490 - facebook/react#28488 - facebook/react#28468 - facebook/react#28321 - facebook/react#28477 - facebook/react#28479 - facebook/react#28480 - facebook/react#28478 - facebook/react#28464 - facebook/react#28475 - facebook/react#28456 - facebook/react#28319 - facebook/react#28345 - facebook/react#28337 - facebook/react#28335 - facebook/react#28466 - facebook/react#28462 - facebook/react#28322 - facebook/react#28444 - facebook/react#28448 - facebook/react#28449 - facebook/react#28446 - facebook/react#28447 - facebook/react#24580 - facebook/react#28514 - facebook/react#28548 - facebook/react#28526 - facebook/react#28515 - facebook/react#28533 - facebook/react#28532 - facebook/react#28531 - facebook/react#28407 - facebook/react#28522 - facebook/react#28538 - facebook/react#28509 - facebook/react#28534 - facebook/react#28527 - facebook/react#28528 - facebook/react#28519 - facebook/react#28411 - facebook/react#28520 - facebook/react#28518 - facebook/react#28493 - facebook/react#28504 - facebook/react#28499 - facebook/react#28501 - facebook/react#28496 - facebook/react#28471 - facebook/react#28351 - facebook/react#28486 - facebook/react#28490 - facebook/react#28488 - facebook/react#28468 - facebook/react#28321 - facebook/react#28477 - facebook/react#28479 - facebook/react#28480 - facebook/react#28478 - facebook/react#28464 - facebook/react#28475 - facebook/react#28456 - facebook/react#28319 - facebook/react#28345 - facebook/react#28337 - facebook/react#28335 - facebook/react#28466 - facebook/react#28462 - facebook/react#28322 - facebook/react#28444 - facebook/react#28448 - facebook/react#28449 - facebook/react#28446 - facebook/react#28447 - facebook/react#24580
- facebook/react#28596 - facebook/react#28625 - facebook/react#28616 - facebook/react#28491 - facebook/react#28583 - facebook/react#28427 - facebook/react#28613 - facebook/react#28599 - facebook/react#28611 - facebook/react#28610 - facebook/react#28606 - facebook/react#28598 - facebook/react#28549 - facebook/react#28557 - facebook/react#28467 - facebook/react#28591 - facebook/react#28459 - facebook/react#28590 - facebook/react#28564 - facebook/react#28582 - facebook/react#28579 - facebook/react#28578 - facebook/react#28521 - facebook/react#28550 - facebook/react#28576 - facebook/react#28577 - facebook/react#28571 - facebook/react#28572 - facebook/react#28560 - facebook/react#28569 - facebook/react#28573 - facebook/react#28546 - facebook/react#28568 - facebook/react#28562 - facebook/react#28566 - facebook/react#28565 - facebook/react#28559 - facebook/react#28508 - facebook/react#20432 - facebook/react#28555 - facebook/react#24730 - facebook/react#28472 - facebook/react#27991 - facebook/react#28514 - facebook/react#28548 - facebook/react#28526 - facebook/react#28515 - facebook/react#28533 - facebook/react#28532 - facebook/react#28531 - facebook/react#28407 - facebook/react#28522 - facebook/react#28538 - facebook/react#28509 - facebook/react#28534 - facebook/react#28527 - facebook/react#28528 - facebook/react#28519 - facebook/react#28411 - facebook/react#28520 - facebook/react#28518 - facebook/react#28493 - facebook/react#28504 - facebook/react#28499 - facebook/react#28501 - facebook/react#28496 - facebook/react#28471 - facebook/react#28351 - facebook/react#28486 - facebook/react#28490 - facebook/react#28488 - facebook/react#28468 - facebook/react#28321 - facebook/react#28477 - facebook/react#28479 - facebook/react#28480 - facebook/react#28478 - facebook/react#28464 - facebook/react#28475 - facebook/react#28456 - facebook/react#28319 - facebook/react#28345 - facebook/react#28337 - facebook/react#28335 - facebook/react#28466 - facebook/react#28462 - facebook/react#28322 - facebook/react#28444 - facebook/react#28448 - facebook/react#28449 - facebook/react#28446 - facebook/react#28447 - facebook/react#24580
… stacks (facebook#28351) `_debugSource` was removed in facebook#28265. This PR migrates DevTools to define `source` for Fiber based on component stacks. This will be done lazily for inspected elements, once user clicks on the element in the tree. `DevToolsComponentStackFrame.js` was just copy-pasted from the implementation in `ReactComponentStackFrame`. Symbolication part is done in facebook#28471 and stacked on this commit.
) Stacked on facebook#28351, please review only the last commit. Top-level description of the approach: 1. Once user selects an element from the tree, frontend asks backend to return the inspected element, this is where we simulate an error happening in `render` function of the component and then we parse the error stack. As an improvement, we should probably migrate from custom implementation of error stack parser to `error-stack-parser` from npm. 2. When frontend receives the inspected element and this object is being propagated, we create a Promise for symbolicated source, which is then passed down to all components, which are using `source`. 3. These components use `use` hook for this promise and are wrapped in Suspense. Caching: 1. For browser extension, we cache Promises based on requested resource + key + column, also added use of `chrome.devtools.inspectedWindow.getResource` API. 2. For standalone case (RN), we cache based on requested resource url, we cache the content of it.
* feat[devtools]: symbolicate source for inspected element ([hoxyq](https://github.com/hoxyq) in [facebook#28471](facebook#28471)) * refactor[devtools]: lazily define source for fiber based on component stacks ([hoxyq](https://github.com/hoxyq) in [facebook#28351](facebook#28351)) * fix[devtools/tree/element]: onClick -> onMouseDown to handle first click correctly ([hoxyq](https://github.com/hoxyq) in [facebook#28486](facebook#28486)) * [DOM] disable legacy mode behind flag ([gnoff](https://github.com/gnoff) in [facebook#28468](facebook#28468)) * Fix Broken Links In Documentation ([justindhillon](https://github.com/justindhillon) in [facebook#28321](facebook#28321)) * Update /link URLs to react.dev ([rickhanlonii](https://github.com/rickhanlonii) in [facebook#28477](facebook#28477)) * [tests] add support for @GATE pragma ([gnoff](https://github.com/gnoff) in [facebook#28479](facebook#28479)) * Devtools: Unwrap Promise in useFormState ([eps1lon](https://github.com/eps1lon) in [facebook#28319](facebook#28319)) * Add support for rendering BigInt ([eps1lon](https://github.com/eps1lon) in [facebook#24580](facebook#24580)) * Include server component names in the componentStack in DEV ([sebmarkbage](https://github.com/sebmarkbage) in [facebook#28415](facebook#28415))
DevTools e2e tests started to fail after landing facebook#28471: - https://app.circleci.com/pipelines/github/facebook/react/50984/workflows/a7be25ed-9547-40e9-87bd-b14d9d2e87da/jobs/798270 - https://app.circleci.com/pipelines/github/facebook/react/50984/workflows/a7be25ed-9547-40e9-87bd-b14d9d2e87da/jobs/798275 - https://app.circleci.com/pipelines/github/facebook/react/50984/workflows/a7be25ed-9547-40e9-87bd-b14d9d2e87da/jobs/798271 - https://app.circleci.com/pipelines/github/facebook/react/50984/workflows/a7be25ed-9547-40e9-87bd-b14d9d2e87da/jobs/798274 - https://app.circleci.com/pipelines/github/facebook/react/50984/workflows/a7be25ed-9547-40e9-87bd-b14d9d2e87da/jobs/798269 There are 2 reasons for that: 1. Versions 16.0 and 16.5 use legacy renderer, which doesn't support source inspection by design: https://github.com/facebook/react/blob/850fac4915864a487e7cb9ecae8a75dbac144174/packages/react-devtools-shared/src/backend/legacy/renderer.js#L831 The corresponding e2e test is now gated for versions >=16.8 2. For other versions (>=16.8), the source is actually `e2e-app-regression.js`, because these regression tests open a different page (not the one we open for tests against React from source) https://github.com/facebook/react/blob/850fac4915864a487e7cb9ecae8a75dbac144174/packages/react-devtools-inline/playwright.config.js#L15-L17
Stacked on #28351, please review only the last commit. Top-level description of the approach: 1. Once user selects an element from the tree, frontend asks backend to return the inspected element, this is where we simulate an error happening in `render` function of the component and then we parse the error stack. As an improvement, we should probably migrate from custom implementation of error stack parser to `error-stack-parser` from npm. 2. When frontend receives the inspected element and this object is being propagated, we create a Promise for symbolicated source, which is then passed down to all components, which are using `source`. 3. These components use `use` hook for this promise and are wrapped in Suspense. Caching: 1. For browser extension, we cache Promises based on requested resource + key + column, also added use of `chrome.devtools.inspectedWindow.getResource` API. 2. For standalone case (RN), we cache based on requested resource url, we cache the content of it. DiffTrain build for commit e528728.
### React upstream changes - facebook/react#28643 - facebook/react#28628 - facebook/react#28361 - facebook/react#28513 - facebook/react#28299 - facebook/react#28617 - facebook/react#28618 - facebook/react#28621 - facebook/react#28614 - facebook/react#28596 - facebook/react#28625 - facebook/react#28616 - facebook/react#28491 - facebook/react#28583 - facebook/react#28427 - facebook/react#28613 - facebook/react#28599 - facebook/react#28611 - facebook/react#28610 - facebook/react#28606 - facebook/react#28598 - facebook/react#28549 - facebook/react#28557 - facebook/react#28467 - facebook/react#28591 - facebook/react#28459 - facebook/react#28590 - facebook/react#28564 - facebook/react#28582 - facebook/react#28579 - facebook/react#28578 - facebook/react#28521 - facebook/react#28550 - facebook/react#28576 - facebook/react#28577 - facebook/react#28571 - facebook/react#28572 - facebook/react#28560 - facebook/react#28569 - facebook/react#28573 - facebook/react#28546 - facebook/react#28568 - facebook/react#28562 - facebook/react#28566 - facebook/react#28565 - facebook/react#28559 - facebook/react#28508 - facebook/react#20432 - facebook/react#28555 - facebook/react#24730 - facebook/react#28472 - facebook/react#27991 - facebook/react#28514 - facebook/react#28548 - facebook/react#28526 - facebook/react#28515 - facebook/react#28533 - facebook/react#28532 - facebook/react#28531 - facebook/react#28407 - facebook/react#28522 - facebook/react#28538 - facebook/react#28509 - facebook/react#28534 - facebook/react#28527 - facebook/react#28528 - facebook/react#28519 - facebook/react#28411 - facebook/react#28520 - facebook/react#28518 - facebook/react#28493 - facebook/react#28504 - facebook/react#28499 - facebook/react#28501 - facebook/react#28496 - facebook/react#28471 - facebook/react#28351 - facebook/react#28486 - facebook/react#28490 - facebook/react#28488 - facebook/react#28468 - facebook/react#28321 - facebook/react#28477 - facebook/react#28479 - facebook/react#28480 - facebook/react#28478 - facebook/react#28464 - facebook/react#28475 - facebook/react#28456 - facebook/react#28319 - facebook/react#28345 - facebook/react#28337 - facebook/react#28335 - facebook/react#28466 - facebook/react#28462 - facebook/react#28322 - facebook/react#28444 - facebook/react#28448 - facebook/react#28449 - facebook/react#28446 - facebook/react#28447 - facebook/react#24580 - facebook/react#28514 - facebook/react#28548 - facebook/react#28526 - facebook/react#28515 - facebook/react#28533 - facebook/react#28532 - facebook/react#28531 - facebook/react#28407 - facebook/react#28522 - facebook/react#28538 - facebook/react#28509 - facebook/react#28534 - facebook/react#28527 - facebook/react#28528 - facebook/react#28519 - facebook/react#28411 - facebook/react#28520 - facebook/react#28518 - facebook/react#28493 - facebook/react#28504 - facebook/react#28499 - facebook/react#28501 - facebook/react#28496 - facebook/react#28471 - facebook/react#28351 - facebook/react#28486 - facebook/react#28490 - facebook/react#28488 - facebook/react#28468 - facebook/react#28321 - facebook/react#28477 - facebook/react#28479 - facebook/react#28480 - facebook/react#28478 - facebook/react#28464 - facebook/react#28475 - facebook/react#28456 - facebook/react#28319 - facebook/react#28345 - facebook/react#28337 - facebook/react#28335 - facebook/react#28466 - facebook/react#28462 - facebook/react#28322 - facebook/react#28444 - facebook/react#28448 - facebook/react#28449 - facebook/react#28446 - facebook/react#28447 - facebook/react#24580
I noticed that there is a delay due to the inspection being split into one part that gets the attribute and another eval that does the inspection. This is a bit hacky and uses temporary global names that are leaky. The timeout was presumably to ensure that the first step had fully propagated but it's slow. As we've learned, it can be throttled, and it isn't a guarantee either way. Instead, we can just consolidate these into a single operation that by-passes the bridge and goes straight to the renderer interface from the eval. I did the same for the viewElementSource helper even though that's not currently in use since #28471 but I think we probably should return to that technique when it's available since it's more reliable than the throw - at least in Chrome. I'm not sure about the status of React Native here. In Firefox, inspecting a function with source maps doesn't seem to work. It doesn't jump to original code.
Stacked on #28351, please review only the last commit.
Top-level description of the approach:
render
function of the component and then we parse the error stack. As an improvement, we should probably migrate from custom implementation of error stack parser toerror-stack-parser
from npm.source
.use
hook for this promise and are wrapped in Suspense.Caching:
chrome.devtools.inspectedWindow.getResource
API.