Skip to content
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

[DevTools] Use line number and column number to match hook #21792

Closed
bvaughn opened this issue Jul 2, 2021 · 2 comments · Fixed by #21833 or #21865
Closed

[DevTools] Use line number and column number to match hook #21792

bvaughn opened this issue Jul 2, 2021 · 2 comments · Fixed by #21833 or #21865

Comments

@bvaughn
Copy link
Contributor

bvaughn commented Jul 2, 2021

DevTools named hook parsing logic currently matches AST nodes using the original line number:

const name = getHookName(
hook,
hookSourceData.originalSourceAST,
((hookSourceData.originalSourceCode: any): string),
((originalSourceLineNumber: any): number),
);

But this may not be sufficient, as mentioned in comment #21641 (comment):

Are we assuming that a line number is sufficient to identify a hook call? Seems like that assumption breaks down in edge cases:

  1. Minified code (either without a source map, or a bundle built from pre-minified inputs) can have multiple hooks on one line.
  2. Some code might be authored with quirky formatting, i.e. not with a typical Prettier / ESLint setup.
@motiz88
Copy link
Contributor

motiz88 commented Jul 2, 2021

While we're on the subject: Column numbers in particular are where a lot of inconsistencies between tools/engines' representations occur and off-by-one errors creep in.

  • V8, SpiderMonkey, JSC all use 1-based lines and 1-based columns in Error.stack.
    • Hermes follows the other engines for the most part but has its own thing going on for bytecode addresses (printed similarly to columns but 0-based). See the Hermes-specific parsing logic in React Native.
  • VS Code uses 1-based lines and 1-based columns.
  • The source-map library (at least AFAIK from older versions) uses 1-based lines and 0-based columns in its API.
  • ESTree/Babel ASTs use 1-based lines and 0-based columns.
  • The Chrome DevTools protocol uses 0-based lines and 0-based columns.

So I suspect we have an incorrect conversion here already - we should fix it and signpost assumptions/conversions more clearly.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 9, 2021

Reopening to address this follow up comment from @motiz88:
#21833 (comment)

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants