DevTools: Update named hooks match to use column number also #21833
Conversation
This prevents edge cases where AST nodes are incorrectly matched.
bvaughn
force-pushed the
bvaughn:devtools-hook-names-column-and-line-number
branch
from
157743a
to
c170b88
|
Comparing: 92af60a...c170b88 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) |
bvaughn
merged commit f52b73f
into
facebook:main
| (line === start.line && column < start.column) || | ||
| (line === end.line && column > end.column) |
motiz88
Jul 9, 2021
Contributor
If I'm not mistaken, column is 1-based here (parsed from Error.prototype.stack) while start.column and end.column are 0-based (coming from the AST).
If I'm not mistaken, column is 1-based here (parsed from Error.prototype.stack) while start.column and end.column are 0-based (coming from the AST).
bvaughn
Jul 9, 2021
•
Author
Contributor
Hey @motiz88 Thanks for checking this PR out.
Line number and column number typically come from the 'source-map' library, but they sometimes come from the 'error-stack-parser' library (if there's no source map):
react/packages/react-devtools-extensions/src/parseHookNames.js
Lines 370 to 387
in
cb8afda
Would we need to add 1 when the values come from the 'source-map' library (so that we could later subtract one here)? Or is it just that they're represented as 0-based within the AST itself, but the value returned by 'source-map' would be 1-based again?
Ideally we'd write a unit test that caught these off-by-one errors to prevent from regressing.
Hey @motiz88 Thanks for checking this PR out.
Line number and column number typically come from the 'source-map' library, but they sometimes come from the 'error-stack-parser' library (if there's no source map):
react/packages/react-devtools-extensions/src/parseHookNames.js
Lines 370 to 387 in cb8afda
Would we need to add 1 when the values come from the 'source-map' library (so that we could later subtract one here)? Or is it just that they're represented as 0-based within the AST itself, but the value returned by 'source-map' would be 1-based again?
Ideally we'd write a unit test that caught these off-by-one errors to prevent from regressing.
bvaughn
Jul 13, 2021
Author
Contributor
If I'm not mistaken, column is 1-based here (parsed from Error.prototype.stack) while start.column and end.column are 0-based (coming from the AST).
Just confirmed this using AST Explorer.
If I'm not mistaken,
columnis 1-based here (parsed fromError.prototype.stack) whilestart.columnandend.columnare 0-based (coming from the AST).
Just confirmed this using AST Explorer.
bvaughn
Jul 13, 2021
Author
Contributor
I think...in practice...this will never cause a problem, because we are matching the 1-based Error stack location for the hook Identifier (e.g. useState) against the 0-based larger VariableDeclarator (e.g. [foo, setFoo] = useState()) so the ranges will always overlap.
I think we should still fix the code, but I'm not sure if it's possible to write an integration test that would fail without this change.
I think...in practice...this will never cause a problem, because we are matching the 1-based Error stack location for the hook Identifier (e.g. useState) against the 0-based larger VariableDeclarator (e.g. [foo, setFoo] = useState()) so the ranges will always overlap.
I think we should still fix the code, but I'm not sure if it's possible to write an integration test that would fail without this change.
This prevents edge cases where AST nodes are incorrectly matched.
Resolves #21792