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: Add ref to inspect element view #21879

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Jul 14, 2021

Summary

Displays the ref of an inspected element if the element has one. Was a bit difficult to track a particular ref through a somewhat larger component tree.

Ref to a class instance
localhost_8080_
Ref to an Element
localhost_8080_ (1)
anonymous callback ref
localhost_8080_ (2)
named callback ref
localhost_8080_ (3)

The UI feels a bit lost since we only have a single value in this new section. Ideally we'd have the key in the view too so we could create a "Misc" view.

On the other hand, the ref will eventually be just another prop (if I understood reactjs/rfcs#107 correctly) so it'll merge with the props view eventually.

Test Plan

  • Tested in shell
  • CI green

if (
(typeof HTMLElement !== 'undefined' && data instanceof HTMLElement) ||
// cross-realm HTMLElement?
/^\[object HTML.+Element\]$/.test(Object.prototype.toString.call(data))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell this is only needed for the shell at the moment. Without this change

function Example() {
  const ref = React.useRef();

  return <React.Fragment><div ref={ref} /><SomeComponent foo={ref} /></React.Fragment>
}

would also crash if one would attempt to inspect SomeComponent since foo.current would not be considered an HTMLElement. I can add the described case to the existing shell to make sure we don't regress when removing the special casing of ref.

@eps1lon eps1lon marked this pull request as ready for review July 14, 2021 14:40
@bvaughn bvaughn self-requested a review July 14, 2021 14:40
@bvaughn bvaughn self-assigned this Jul 14, 2021
@bvaughn
Copy link
Contributor

bvaughn commented Jul 14, 2021

I think I dig this change overall. I'm going to poke at the UI a little. May have a nit or two.

@bvaughn
Copy link
Contributor

bvaughn commented Jul 14, 2021

The UI feels a bit lost since we only have a single value in this new section. Ideally we'd have the key in the view too so we could create a "Misc" view.

The key is shown as part of the name up top, e.g. the little "3" to the left of the name:
Screen Shot 2021-07-14 at 3 32 02 PM

On the other hand, the ref will eventually be just another prop (if I understood reactjs/rfcs#107 correctly) so it'll merge with the props view eventually.

Yeah. Makes me wonder if we should just show it inline, within the props panel...but maybe this would be confusing to people, since it's not editable?

@eps1lon
Copy link
Collaborator Author

eps1lon commented Jul 14, 2021

Yeah. Makes me wonder if we should just show it inline, within the props panel...but maybe this would be confusing to people, since it's not editable?

Another idea would be to have it under "Props" but a little bit separate (e.g. with a horizontal divider).

Though my initial idea also was to just put them in the props. My concern was not really about editable vs not editable but that ref needs a special API to work "like" a prop: React.forwardRef.

The key is shown as part of the name up top, e.g. the little "3" to the left of the name:

I always miss that, thanks!.

@@ -131,6 +132,13 @@ export default function InspectedElementView({
store={store}
/>

<InspectedElementRefTree
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do keep ref as a separate panel, I think it should be above "props" because it's kind of a top-level thing (like key)

Copy link
Collaborator Author

@eps1lon eps1lon Jul 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just marking this unresolved for visibility.

I always view the order in "what is most important during debugging" and I don't think ref is more important than other props. Case in point: There hasn't been any request to expose the ref in devtools.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There hasn't been any request to expose the ref in devtools.

That's a good point!

Although at the same time, sometimes people don't know that it's something they could have asked for.

@bvaughn
Copy link
Contributor

bvaughn commented Jul 14, 2021

Displays the ref of an inspected element if the element has one. Was a bit difficult to track a particular ref through a somewhat larger component tree.

Can you tell me a little more about this case?

@bvaughn
Copy link
Contributor

bvaughn commented Jul 14, 2021

If we land a change like this, I think I'm leaning towards showing these special not-quite-props in the "props" section, since their values get passed in like props:
Screen Shot 2021-07-14 at 5 09 17 PM

@bvaughn
Copy link
Contributor

bvaughn commented Jul 14, 2021

What if I added a new section called "special props" (to match the React docs) and it had "key" and "ref" ?

@bvaughn
Copy link
Contributor

bvaughn commented Jul 14, 2021

@eps1lon How do you feel about an alternative like this?
Screen Shot 2021-07-14 at 5 38 46 PM

Key may not make the cut here. Despite what the docs say, Sebastian thinks it's more about the parent than the currently-selected element (and I see this point)

@ryota-murakami
Copy link
Contributor

This Twitter vote seems like here's context 🗳
https://twitter.com/brian_d_vaughn/status/1415533461583147009

@eps1lon
Copy link
Collaborator Author

eps1lon commented Jul 15, 2021

Can you tell me a little more about this case?

Screenshot from 2021-07-15 08-31-09

I had some trouble making sure that nodeRef on PickersCalendarSlideTransition was passed as a ref to PickersCalendarWeekContainer and vice versa.
In another instance I had a forwardRef component that rendered div > div and I wanted to make sure the ref is passed to the second div.

All of these things can be done in code but I'm relying more and more on devtools for analyzing which prop goes where. ref not being part of that story slows things down quite a bit since I now have to switch between IDE and devtools.

@eps1lon How do you feel about an alternative like this?

That seems fine to me. Though I don't have any strong opinions about either variant (props, special props, with key, without key) etc since I'm comfortable distinguishing their semantics. I think we'll see over time if the UI is confusing or not.

Comment on lines +3449 to +3719
// Ref is a special case;
// don't dehydrate it in the same way (because it's not hydratable/inspectable).
// Just stringify it instead...
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is ref different than a prop holding a ref? For example, before React.forwardRef we often had a custom prop like forwardedRef.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good question. Ref is different (at least currently) because it has special React semantics. But you're right, a custom prop name like forwardedRef would be inspectable and would show Fiber internals. Maybe we need a more robust solution for filtering keys like that on the DevTools side. 🤔 Could be a follow up to consider.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also to consider: The ref might not point to a DOM element but point to imperative methods or other mutable values.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My gut feeling was that stringifying is still okay in that case, but maybe...I don't know. I don't really know if I believe in this change overall yet.

@bvaughn
Copy link
Contributor

bvaughn commented Jul 15, 2021

All of these things can be done in code but I'm relying more and more on devtools for analyzing which prop goes where.

This is nice to hear 😄

Some feedback from Rachel and Dan led me to try a slight variation here (basically just getting rid of the "special props" header):
Screen Shot 2021-07-15 at 9 52 48 AM

Feedback from Sebastian has me considering dropping key too and just showing ref here.

@eps1lon
Copy link
Collaborator Author

eps1lon commented Jul 15, 2021

Some feedback from Rachel and Dan led me to try a slight variation here (basically just getting rid of the "special props" header):

I like that. But we need to make sure that no other component with a header can be rendered in between (in the future).

@bvaughn
Copy link
Contributor

bvaughn commented Jul 15, 2021

I think I might just remove key and combine the ref and HOC badges into a single panel up top.

Current UI:
Screen Shot 2021-07-15 at 4 21 30 PM

Proposed change:
Screen Shot 2021-07-15 at 4 12 12 PM

Screen Shot 2021-07-15 at 4 12 28 PM

Dan shared a concern (that I agree with) about showing "hocs" as a props-like-thing-that-has-a-name so maybe this mock isn't going to fly.

@bvaughn
Copy link
Contributor

bvaughn commented Jul 15, 2021

@gaearon said: #21796 (comment)

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.

Yes and no. At least React.forwardRef is going to be around for a while. And React.memo too for some cases.

@bvaughn
Copy link
Contributor

bvaughn commented Jul 16, 2021

Okay. I split refs and hocs into separate panels.
Screen Shot 2021-07-15 at 11 36 37 PM

Looks a little weird when there's both, but I guess not too common.

@ryota-murakami
Copy link
Contributor

I think most DOM related ref is used to escape hatch for getting access raw DOM API.
So ref behavior programed in mostly useEffect/customHook that is not appeared on React Element tree.
I think good idea showing ref label explicitly and it's great I could [go to definition] from ref label(click,dBclick, etc...).
Destination is devlools source tab that provide sourcemaped original code, or just jump user editor.
I think read real code is pretty much well option to understanding what does the 'ref' accutualy doing from the ''ref" label.

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

Successfully merging this pull request may close these issues.

None yet

5 participants