[WIP] DevTools: Support named hooks for >1 module in a bundle

[motiz88]

Summary

As pointed out in #21641 (comment), the named hooks feature currently assumes that the source code to parse appears in the first file of the source map. This is a WIP branch where I'll attempt to fix this bug in preparation for #21782.

(This PR may evolve to include more work related to #21782 before it's reviewable, in which case I'll update the summary and test plan.)

Resolves #21794.

Progress / implementation plan

• f260bcf adds a simple bundle built with Rollup that is effectively equivalent to __source__/__compiled__/external/Example.js (i.e. the source map has a single file so this should work).
  • The test is failing with "Hook source code location not found." but I'm not sure why - cc @bvaughn, want to help debug this? (EDIT: Fixed by 556708c)
  • The test is now failing with a different error. (EDIT: Fixed by 8d8dd25)
  • The test is passing!
• 556708c stops Jest's automatic source map support from interfering with the tests.
• 8d8dd25 changes source map fetching to be relative to the script's path/URL.
  • Test this manually in the browser to ensure correctness.
  • More rigorous URL/path parsing? We're doing some pretty ad-hoc stuff at the moment.
• e0aa1af adds more modules to the test bundle so we can observe the bug - the tests fail due to parsing the wrong source file.
• af05660 derives originalSourceAST from the source URL returned by the source map, and updates several assumptions that this violates in the existing implementation.
  • This can probably do with another round of refactoring. I've added TODO comments related to this.

Test Plan

yarn test --build --project devtools

[sizebot]

Comparing: ed6c091...c4e3620

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	127.04 kB	127.04 kB	=	40.72 kB	40.72 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	129.85 kB	129.85 kB	=	41.63 kB	41.63 kB
facebook-www/ReactDOM-prod.classic.js	=	404.24 kB	404.24 kB	=	74.80 kB	74.80 kB
facebook-www/ReactDOM-prod.modern.js	=	392.94 kB	392.94 kB	=	73.06 kB	73.06 kB
facebook-www/ReactDOMForked-prod.classic.js	=	404.24 kB	404.24 kB	=	74.80 kB	74.80 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against c4e3620

[bvaughn]

the named hooks feature currently assumes that the source code to parse appears in the first file of the source map. This is a WIP branch where I'll attempt to fix this bug in preparation for #21782.

Oooh nice! 😁 This was something I meant to follow up on before landing but then put it off. Thanks for taking a look!

[bvaughn]

Probably refactor the parsing logic to happen lazily after the call to SourceMapConsumer.originalPositionFor (which returns the right source file to parse).

I assume you're referring to the branch where we have source maps? (It's probably still fine to parse in parseSourceAST for the no-source-map case?

[motiz88]

It's probably still fine to parse in parseSourceAST for the no-source-map case

Guess so. I haven't looked closely at the perf implications or what the necessary refactoring is - do you think we'll gain anything by treating these two cases differently? Naively I would like to unify them just to keep complexity down. (Especially since we'll have yet another branch when we add hook metadata directly to the source map.)

[motiz88]

I think something in the current test harness is applying source maps when it shouldn't. The "Hook source code location not found" error happens because ReactDebugHooks thinks the example component has the name useState. The error goes away if I delete index.js.map and restart Jest.

Specifically, this seems like a case of using the identifier names in a source maps to try to approximate the local function name given a stack frame pointing to (an arbitrary line in the body of the function), which is a classic mis-application of source maps. (Incidentally this is exactly the problem that function maps solve.) So whatever package is doing this stack frame translation has a bug.

Anyway, AFAICT the intention was to disable all "magic" source map translation in this test in order to match browser behaviour, but that doesn't seem to be the case for these inputs.

[bvaughn]

Anyway, AFAICT the intention was to disable all "magic" source map translation in this test in order to match browser behaviour, but that doesn't seem to be the case for these inputs.

Yes. I lost several days trying to figure out why this was happening. Sometimes it would stop happening but then restart again whenever I e.g. connected Node to the Chrome debugger.

Deleting prepareStackTrace was an attempt to prevent Jest from automatically mapping stack frame line numbers, but it doesn't seem to be working:

react/packages/react-devtools-extensions/src/__tests__/parseHookNames-test.js

Lines 34 to 37 in ed6c091

	// Jest (jest-runner?) configures Errors to automatically account for source maps.
	// This changes behavior between our tests and the browser.
	// To "fix" this, clear the prepareStackTrace() method on the Error object.
	delete Error.prepareStackTrace;

I have no idea how to get around this at the moment. I'll try reaching out on Twitter to see if anyone else has ideas.

[motiz88]

Attached a Node debugger and found something interesting. Internally in Node, there is another copy of prepareStackTrace that lives on something called MainContextError. In our case, Error.prepareStackTrace is deleted but MainContextError.prepareStackTrace is still there (and is the Jest one). I'll dig some more and figure out the best way to work around this.

[bvaughn]

Attached a Node debugger and found something interesting. Internally in Node, there is another copy of prepareStackTrace that lives on something called MainContextError. In our case, Error.prepareStackTrace is deleted but MainContextError.prepareStackTrace is still there (and is the Jest one). I'll dig some more and figure out the best way to work around this.

Great find.

I wonder– if we can't figure out how to disable this, would we be able to reverse it by providing our own Error.parseStackTrace (using the source-map originalPositionFor(generatedPosition))?

[motiz88]

1. I think the prepareStackTrace patching issue is behind us as of 556708c.
2. With the fix for relative paths in 8d8dd25 (which I think is correct, listed some followups above) the single-module bundle test is passing 🎉 which means I can now add a failing multi-module test and figure out the logic changes.

[bvaughn]

Great find! 👏🏼

[motiz88]

I have this nearly working, but I'm still fighting one of the test cases and my code is a mess. I need to come back to this with fresh eyes next week. Heads up that I won't have a lot of bandwidth next week and I'm going on PTO afterwards, so this may end up waiting a while. Hopefully I can push a minimally working version sooner rather than later.

This is doable without lazifying the parsing, but still involves a bunch of refactoring as we're changing prior assumptions re: what's cacheable and with what keys.

[bvaughn]

Thanks for the update and no worries. I can always pick this up and iterate on it myself while you're out.

[motiz88]

Oh and since I mentioned caching changes, here's something else to think about (which may warrant its own issue): How does this all work with Fast Refresh? A new compiled script can be loaded into the VM that has the same URL (hookSource.fileName), same source map URL and even the same original source URL(s) as a previously loaded script, but contains completely different code.

We may want to just invalidate the world during Fast Refresh. This would work, assuming no two versions of the same script can hang around in the React tree simultaneously. (Does Fast Refresh guarantee this? I vaguely remember some limitations around memoised props, but maybe when it comes to stale Hooks specifically we have a stronger guarantee?) We could also try to invalidate more granularly for better performance.

[bvaughn]

Oh and since I mentioned caching changes, here's something else to think about (which may warrant its own issue): How does this all work with Fast Refresh? A new compiled script can be loaded into the VM that has the same URL (hookSource.fileName), same source map URL and even the same original source URL(s) as a previously loaded script, but contains completely different code.

There's maybe some follow-up work we could do for handling Fast Refresh / hot-reloading in general, but I'm not overly concerned about that for initial launch.

I've filed a follow up task here: #21796

It's going to be a little tricky since DevTools doesn't have a signal about when Fast Refresh has occurred.

[bvaughn]

@motiz88 If you could, tag me with an explicit hand-off comment when you think you're done working on this for a while. I'll be happy to pick up.

[motiz88]

@bvaughn Tests are passing now and the implementation is somewhat coherent. I think realistically I'm done with this until after I'm back from PTO (however tempting it is to keep going 😅). Happy to discuss next steps and/or elaborate on the code as needed, but the branch is yours now.

[bvaughn]

Thanks Moti. I'll dig in on Tuesday.

[bvaughn]

I verified the before and after (broken then fixed) behavior using a local create-react-app TODO list demo application. I'm going to go ahead and land this PR and continue working in follow ups. Thank you so much for the contributions @motiz88 🙇🏼