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] Improve Fast Refresh support for named hook detection #21796

Closed
bvaughn opened this issue Jul 3, 2021 · 8 comments · Fixed by #21891
Closed

[DevTools] Improve Fast Refresh support for named hook detection #21796

bvaughn opened this issue Jul 3, 2021 · 8 comments · Fixed by #21891

Comments

@bvaughn
Copy link
Contributor

bvaughn commented Jul 3, 2021

#21641 added support to DevTools for showing hook names for the inspected component. For performance reasons, this feature caches source maps and ASTs by file name to avoid re-parsing any time a new component is inspected.

As #21790 (comment) mentions:

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.

So our cache may be incorrect in the case of Fast Refresh (or other hot-reloading system).

The comment goes on to say:

We may want to just invalidate the world during Fast Refresh.

The trouble with this idea is that DevTools is not explicitly notified when Fast Refresh reloads a component.

The React core team has talked about the idea of adding a new, DEV-only Remount bit/flag to Fibers to notify DevTools of this case. That would be a helpful signal here too that any cached source/AST should be cleared when hooks are next inspected. (The flag would need to be passed from backend to frontend as part of the "inspected element" payload.)

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 15, 2021

To repro this case...

  1. Use create-react-app to create a new app
  2. Start the DEV mode server (yarn start)
  3. Add the following content to the App.js file:
import React, { useState } from 'react'

export default function Example() {
  const [count, setCount] = useState(0);

  return count;
}
  1. Open DevTools, select the component, and load hook names
  2. Now edit the content:
import React, { useState } from 'react'

export default function Example() {
  const [bool, setBool] = useState(true);
  const [count, setCount] = useState(0);

  return bool + count;
}
  1. Re-select the component after Fast Refresh updates the page
  2. DevTools will now show two hooks: the first one (incorrectly) named as "count" and the second one without a name (due to a cached AST).

Reloading the page will not fix this issue. Fixing this issue requires closing and re-opening DevTools.

@bvaughn bvaughn self-assigned this Jul 15, 2021
@gaearon
Copy link
Collaborator

gaearon commented Jul 15, 2021

The trouble with this idea is that DevTools is not explicitly notified when Fast Refresh reloads a component.

Fast Refresh Runtime calls scheduleRefresh on the injected renderer interface. Can DevTools wrap/intercept that?

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 15, 2021

The trouble with this idea is that DevTools is not explicitly notified when Fast Refresh reloads a component.

Fast Refresh Runtime calls scheduleRefresh on the injected renderer interface. Can DevTools wrap/intercept that?

That's an interesting idea!

Let me look into that. 😄

@gaearon
Copy link
Collaborator

gaearon commented Jul 15, 2021

"hocs" are kind of unfortunate as the concept is already confusing enough, and here we have an abbreviation. Unlike key or ref or real props it also doesn't correspond to any name in the code. I was kind of liking that we don't give it an explicit name and just show a badge.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 15, 2021

"hocs" are kind of unfortunate as the concept is already confusing enough, and here we have an abbreviation. Unlike key or ref or real props it also doesn't correspond to any name in the code. I was kind of liking that we don't give it an explicit name and just show a badge.

This was my reservation too, although I also really disliked seeing a HOC strip and a "ref" strip as separate panels. This mock/POC was trying to combine them a little.

I think the concern about showing "hocs" like as a props-like-named-key is good though :( Hmmm.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 15, 2021

Also LOL I just realized that I left my previous comment on the wrong issue. I am so bad at context switching.

@gaearon
Copy link
Collaborator

gaearon commented Jul 15, 2021

I also really disliked seeing a HOC strip and a "ref" strip as separate panels

HOCs are kind of fading away though? I don't know if they're even that common in practice in modern codebases.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 15, 2021

Let's move this chat to the right thread 😅 #21879

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

Successfully merging a pull request may close this issue.

2 participants