bvaughn / react-devtools-experimental

Experimental rewrite of the React DevTools extension
https://react-devtools-experimental.now.sh
MIT License
965 stars 55 forks source link

Enhancement: reflect over objects for their fields and values recursively #294

Closed vnvizitiu closed 5 years ago

vnvizitiu commented 5 years ago

Hello,

I would like to propose if possible to try and reflect over the fields of the objects in the inspector like the released react dev tools do.

As an example this is from the latest version of the experimental tool:

image

And this is the one from the released dev tools for the same instance:

image

Thank you, Vlad V.

bvaughn commented 5 years ago

Can you share the app/code that these screenshots came from?

vnvizitiu commented 5 years ago

Hello,

I attached a gdrive link (didn't see how to attach a zip to a comment if possible) to a CRA application that reproduces the scenario (couldn't send the product source code), it can be found here: https://drive.google.com/file/d/1M6bb6cj1pL0bQ1CFYkrIvEsMdj9Yg0aj/view?usp=sharing

This is a side by side comparison between the experimental devtools on the right in chrome and the released devtools in firefox on the left.

image

Also I noticed some other enhancements though you can let me know if they are worth it, if i should make different items for them or none at all. The other minor enhancements features that are in the released version are as follows:

Thanks for the good support and quick turnaround 😃 Vlad V.

bvaughn commented 5 years ago

Quick follow up to the additional things you mention:

the breadcrums at the bottom of the left hand side, would be nice to have that to see the nesting of a components including html tags like divs

The breadcrumb feature was intentionally omitted from the new DevTools. I don't think we're open to adding it back in for the time being.

Also based on this maybe an XPath style filter would also be awesome 😃

I see how this would be useful, but it would likely be too slow to build in a performant way (so I probably wouldn't be open to adding that change). If you feel strongly about it, I'd suggest maybe a PR with a proof of concept implementation as a good next step.

when doubleclicking the name of a field in the inspector (for example doubleclicking the directObject prop, or the _number field under customObject) allows you to select and copy it so that you can paste it in your code in non-typescript scenarios, where a typo can cause some amount of grief.

Double clicking expands/collapses nested props. For copying, there is a dedicated button: Screen Shot 2019-05-31 at 8 59 35 AM

bvaughn commented 5 years ago

Code Sandbox reduction of the code above: https://codesandbox.io/s/jovial-lichterman-px7pw

bvaughn commented 5 years ago

Looks like this is caused by our dehydration logic: https://github.com/bvaughn/react-devtools-experimental/blob/c0f686eed9bf18d3015160229145e829f31a66cc/src/hydration.js#L197-L203

I believe we could remove the if branch entirely and just treat custom objects the same, but it would require some testing. Quickly trying it locally though shows the custom object filed: Screen Shot 2019-05-31 at 9 19 49 AM

We should also expand on our InspectableElements test app to cover custom objects like this.

gaearon commented 5 years ago

I'm also seeing this on the React website:

Screen Shot 2019-06-12 at 15 30 21

There are two issues here.

  1. We don't go into the array :-(
  2. The string is displayed without quotes. And empty string becomes just (string) which doesn't necessarily imply it's empty (I thought we just fail to display it). I would prefer if our display for strings was similar to Chrome's — where quotes are part of the string, and where you can omit them to get literals like 42. If you type something invalid, it resets to null.
bvaughn commented 5 years ago

I'm also seeing this on the React website:

Where? All of the Arrays I'm seeing are working

Ah, TODO list. Nm.

bvaughn commented 5 years ago

Ah, you're only seeing Array instead of the actual contents because that specific Array is empty. If you add an item to it, it will expand:

Screen Shot 2019-06-12 at 3 41 28 PM

bvaughn commented 5 years ago

Support for custom objects/fields added in 96f7646.

Fix has been deployed. Re-install to test it!

bvaughn commented 5 years ago

The string is displayed without quotes.

This was intentional. I'll take another look at it to see if I still prefer this, but I think I might.

Edit I think the current behavior still feels fine to me 😄 Maybe share a POC if you disagree?

And empty string becomes just (string) which doesn't necessarily imply it's empty (I thought we just fail to display it).

Do you think it would be more intuitive if it said "(empty string)" ?

I would prefer if our display for strings was similar to Chrome's

We do :grin: I think the closest Chrome equivalent UI is the Style panel, which doesn't show quotation marks: Screen Shot 2019-06-12 at 3 49 59 PM

where quotes are part of the string, and where you can omit them to get literals like 42. If you type something invalid, it resets to null.

This was slightly trickier to implement, and could result in annoying "invalid" states if you omitted a quotation mark when editing, so it felt strictly more cumbersome to edit values.

gaearon commented 5 years ago

I guess I’m not sure what to do with a prop that can either be a string or a number. The “style editor” is different because it has no concept of different types. But JavaScript does. That’s why JS object editor shows quotes (and thus lets you change the type or input null/undefined).

gaearon commented 5 years ago

Do you think it would be more intuitive if it said "(empty string)" ?

Yes. (Assuming we don’t want to allow changing its type — in which case we’d need to do the quotes thing.)

bvaughn commented 5 years ago

I don't see changing type between number/string as a common case, and I think I'd prefer the editing experience to be smooth for the common case. (TBH I don't even see editing values to be that common in the first place, but within that subset...)

Feel free to share a POC if you have concrete ideas. 😄 I built this the way it made the most sense for me, and I kind of like it as is, but I'm open to looking at new ideas.

vnvizitiu commented 5 years ago

Confirmed on my end as well, both for local custom objects and Moment objects (where it was first encountered) 🥇

gaearon commented 5 years ago

I don't see changing type between number/string as a common case

It’s not that even so much about changing type as about having no hint about the type from looking at the inspector. There may be real bugs in components if for example they assume a number but get a string. if (props.kind === SomeEnum.Foo) would fail if kind is accidentally a string but should’ve been a number. But DevTools give no indication as to whether a prop is a number or an equivalent string. Or even if it’s null or a string “null”. Etc. So DevTools don’t help you to find the problem in this case. But they used to help with that. And so does Chrome’s display of objects. That’s my concern.

bvaughn commented 5 years ago

But DevTools give no indication as to whether a prop is a number or an equivalent string.

That's not really true... DevTools either uses a <input type="text"> or an <input type="number"> in that case, which is displayed differently by the browser. (The latter has increment/decrement buttons.)

But anyway, again, please share a POC if you have one...

gaearon commented 5 years ago

DevTools either uses a or an in that case, which is displayed differently by the browser. (The latter has increment/decrement buttons.)

Ah ok, I missed that.

But anyway, again, please share a POC if you have one...

I get that you're saying "if you think this is important, please do it yourself because I don't think it is". :-) I don't meant to pressure you into implementing it or anything. Just debating what it should behave like first. If you don't think this debate is useful without actually intending to implement it, I can stop these suggestions.

gaearon commented 5 years ago

One thing I've noticed is that inner keys (at least in props pane) seem to use quotes for strings:

Screen Shot 2019-06-13 at 3 09 44 PM Screen Shot 2019-06-13 at 3 10 00 PM

Is it because they're not editable? Maybe this is what tripped me — sometimes strings are in quotes, and sometimes they're not.

bvaughn commented 5 years ago

😆 That's not quite what I'm saying. I've been trying to say that I think the current UI is pretty good, and I did put some thought into things like displaying quotation marks when I originally built it. It could probably be better (everything could) but I don't have concrete ideas for how and I don't think it's the most important way to use our time either, so I'm just trying to be explicit about leaving the ball in your court if this is something you're passionate about 😄

Yes, non-editable values display with quotes.

bvaughn commented 5 years ago

I filed issue #321 as a follow up for the things you've mentioned.