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

Add a checkbox to hide HOC wrappers in tree-view #503

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
@zackargyle

zackargyle commented Feb 10, 2017

A pain point I've shared with many others is the congestion of the devtools tree when composing a large app with many HOCs. This PR introduces a "Hide Wrappers" checkbox that will hide all component wrappers from the tree view, instead showing only the tree of components that render at least some DOM content.

An HOC is determined to be any node that only returns a single child that is a non-DOM component.

Happy to talk about better ways of doing this, but I'd love to have something like available!

devtools

@gaearon

This comment has been minimized.

Member

gaearon commented Feb 10, 2017

I'd really want to get something like this in, but I think it needs more work.

An HOC is determined to be any node that only returns a single child that is a non-DOM component.

I don’t think this is a very good proxy. Any custom component that renders to another component would do that (which is very common in large apps).

Instead, I would like to see blackboxing by regex. We could provide a reasonable default (e.g. components with ( and ) in their names).

There should also be a way to “discover” components are being blackboxed. For example, a ... in the tree. Otherwise it’s easy to forget they’re being blackboxed.

@zackargyle

This comment has been minimized.

zackargyle commented Feb 10, 2017

@gaearon
I guess part of the issue is deciding what exactly we are wanting to solve. I used the word HOC in the description, but in the code I'm specifically referring to any "wrapper". Custom components that render nothing but another component, are "wrappers" (and a kind of static HOC).

When I want to hide wrappers it's because I want to only see the components rendering actual DOM content. There might be a more generic/extensible/thorough way to do this, which I'm happy to discuss. But I'm glad we can start figuring out a solution.

I also think it's valuable not to enforce a naming convention if possible.

Hopefully others can weigh in their thoughts!

@gaearon

This comment has been minimized.

Member

gaearon commented Feb 10, 2017

Custom components that render nothing but another component, are "wrappers" (and a kind of static HOC).

I would not agree with this. For example, if you're using a company-wide component library with <Button>, <Option>, etc, it would be confusing to not see your components (e.g. UserPanel) while seeing all the low-level components you never touch.

Blacklisting is useful, but I don’t think this is the right heuristic.

@kyleshevlin

This comment has been minimized.

Contributor

kyleshevlin commented Feb 10, 2017

I am torn regarding this. The simplicity of a checkbox to do this is enticing, but personally I feel it's important that the React DOM tree always reflect how it's different than the actual DOM. Thus, I am with @gaearon in desiring some UI feature to indicate the existence of HOCs or wrapping components. Perhaps instead of hiding, they were collapsed to ... until a non-HOC was found.

@blainekasten

This comment has been minimized.

blainekasten commented Feb 10, 2017

Is it adding too much complexity / confusion to have both?

@gaearon

This comment has been minimized.

Member

gaearon commented Feb 10, 2017

To have both of what? I’m pretty sure, no matter what heuristic we use, ... is important because otherwise the feature is undiscoverable once it’s on.

@bloodyowl

This comment has been minimized.

bloodyowl commented Feb 10, 2017

I think that two options would be better than just a toggle:

  • show wrappers
  • show DOM nodes

which would be checked by default.

in most of my use cases, I don't care at all about DOM nodes but I'm more interested in just seeing the HOC.

@kyleshevlin

This comment has been minimized.

Contributor

kyleshevlin commented Feb 10, 2017

@bloodyowl's idea is an interesting suggestion. HOCs are generally passing props into presentational components. Having the option to quickly hide or show both would make it easy to quickly inspect container or presentational components.

@zackargyle

This comment has been minimized.

zackargyle commented Feb 10, 2017

The current implementation will only show presentational components and hide everything else.

@calebmer

This comment has been minimized.

calebmer commented Feb 10, 2017

Could you perhaps only render components in a user's source directory?

@gaearon

This comment has been minimized.

Member

gaearon commented Feb 10, 2017

There is no concept of “presentational” components (or even “wrappers”) in React itself, and even if we were to use this distinction, it’s not what PR does (RedButton that renders a Button is no less “presentational”).

I don’t think this is the right approach to solving this issue. I don’t think we can find a good heuristic for this.

Browsers already support a very similar feature for callstacks: blackboxing. I think we should learn from that UI and apply the lessons here. It should be configurable and toggleable, just like in existing stack blackboxing UIs.

@jtadmor

This comment has been minimized.

jtadmor commented Feb 10, 2017

It sounds like there's a need for more than one simple control. If there's differing opinions on what people want / need, can we just a more complicated input than a checkbox?

@jquense

This comment has been minimized.

Contributor

jquense commented Feb 10, 2017

Im so excited to see this! I mostly don't use the devtools anymore because the component stack is so deep it's silly to think of it s a tree :P

I generally find I want to hide components for a two reasons:

  • I only want to see my components, eg not the 4 levels of HOCs my UI library adds to button. (seems hard to do with file info?)
  • I want to see components that render DOM nodes to understand where markup is coming from

so I agree it'd be nice to configure but I don't think a regex would cut it. Especially since app components usually wrap library components of the same name: Button -> ReactBootstrap.Button. I also think the HOC heuristic here is pretty good, but yes apps usually have tons of single child composed components that aren't the same sort of thing as like redux's connect()

Sorry I don't have a proposal! just weighing tho :)

@gaearon

This comment has been minimized.

Member

gaearon commented Feb 10, 2017

@jtadmor

It sounds like there's a need for more than one simple control. If there's differing opinions on what people want / need, can we just a more complicated input than a checkbox?

In my previous comment I linked to an example showing how this could be done: blackboxing. Have you had a chance to check it out? I think we need to support something very similar (just replace "call frame" with "component" there).

So, to extrapolate from that article, we could have:

  • A list of regexes as actual configuration format
  • A context menu to quickly add a component to the blackbox
  • A visual indicator that blackboxing is active
  • An ability to skip blackboxing when necessary
@drcmda

This comment has been minimized.

drcmda commented Feb 10, 2017

An option that would cut down on wrappers, even if that's a vague concept, would help a lot. Often our components are wrapped either in redux connects or style wrappers, or both. It produces an overwhelming tree that doesn't resemble the JSX in the editor (using decorators).

I find the definition of a HOC above reasonable. Showing components that produce dom as a filter would go a long way in taming the often noisy output.

@jquense

This comment has been minimized.

Contributor

jquense commented Feb 10, 2017

one, I think, important difference between the chrome blackboxing and component BBing tho. Its file based, vs say individual functions. Which is nice when you want to not see anything from node_modules, or from bootstrap.js. In this case if the regex is against displayName then it's gonna be hard to block out the implementation details of libraries and the like. It'll be to difficult to construct a regex that's helpful

@gaearon

This comment has been minimized.

Member

gaearon commented Feb 10, 2017

then it's gonna be hard to block out the implementation details of libraries and the like.

One interesting thing @sebmarkbage suggested off the thread is we might be able to use element._owner to track implementation details of blacklisted libraries. It wouldn't be 100% foolproof though for some APIs.

@gaearon

This comment has been minimized.

Member

gaearon commented Feb 10, 2017

Another interesting direction is using element.__source (which gives us filename and location if you apply the right Babel plugin) for blackboxing.

@jtadmor

This comment has been minimized.

jtadmor commented Feb 10, 2017

@gaearon

In my previous comment I linked to an example showing how this could be done: blackboxing. Have you had a chance to check it out? I think we need to support something very similar (just replace "call frame" with "component" there).

Another interesting direction is using element.__source (which gives us filename and location if you apply the right Babel plugin) for blackboxing.

This sounds correct and these might be good approaches.

Like others, I sometimes find that I ONLY want to see a particular HOC to introspect their local state. Sometimes I want to see just the DOM-rendering components.

I'd love to see an approach that included regex-based source / owner / componentName filtering, but also basic heuristics like "does not render DOM" and "does render DOM" seem like they would be quite handy.

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Feb 10, 2017

Oh I like the __source suggestion. That's definitely better than owner.

@zackargyle

This comment has been minimized.

zackargyle commented Feb 10, 2017

@sebmarkbage How would we detect internal HOCs with __source?

I'd be down for something simple like:

  • Right click on node and click Hide components of this type
  • Node gets added to blacklist and stored in localstorage (or some persistent storage)
  • Hide blacklisted types checkbox is marked as true

Then you could hide anything you want, have it persisted, and be able to turn it off with a checkbox.

@pixel9

This comment has been minimized.

pixel9 commented Feb 10, 2017

How about adjusting the visual styling so that HOC's have less (or more) contrast based on the various heuristics suggested above?

@gaearon

This comment has been minimized.

Member

gaearon commented Feb 10, 2017

The problem with __source is you don't get it for third party libraries since they wouldn't contain the annotations for size. So it's shallow for third party code.

@gaearon

This comment has been minimized.

Member

gaearon commented Feb 15, 2017

@zackargyle Are you interested in trying to implement some of the ideas in this thread? I don’t think we’ll proceed with this particular version, but a more generic version of blackboxing would be amazing.

@zackargyle

This comment has been minimized.

zackargyle commented Feb 15, 2017

@gaearon Yeah I can put something together, unless someone else would like to try. Got caught up with some work stuff, but I can work on it later this week. Agreed that it would be a great feature to have.

@jaredly

This comment has been minimized.

Contributor

jaredly commented Feb 16, 2017

I'm taking a whack at this

@zackargyle

This comment has been minimized.

zackargyle commented Feb 16, 2017

@jaredly Right on!

@jaredly

This comment has been minimized.

Contributor

jaredly commented Feb 16, 2017

hmm hiding things from display is super easy, but the navigation code is not clean at all :( :( :( I think it wants some refactoring

@zackargyle

This comment has been minimized.

zackargyle commented Mar 24, 2017

@jaredly any luck on this? Would still love to have something!

@gaearon

This comment has been minimized.

Member

gaearon commented Apr 19, 2017

kind ping @jaredly

@kof

This comment has been minimized.

kof commented Apr 19, 2017

This issue has a huge importance for general HOC usage in the react community. Lots of people hesitate to use them because they introduce noise in the dev tools tree.

@gaearon

This comment has been minimized.

Member

gaearon commented Apr 19, 2017

Do you want to work on a more generic exclude mechanism?

@kof

This comment has been minimized.

kof commented Apr 19, 2017

@gaearon me? I just recently discovered both issues, didn't think of any implementation details.

@gaearon

This comment has been minimized.

Member

gaearon commented Apr 19, 2017

Going to close this particular PR, but happy to see more generic solutions in #604.

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