[WIP] Support hiding components in devtools #1187

Gvozd · 2018-10-15T19:40:02Z

Add two options for hiding components

For components with [Symbol.for('react.devtools.hide')] = true - rebase of PR Support hiding components in devtools with Symbol.for('react.devtools.hide') #997
If component define displayName - I think that most popular case for HOC

I'm waiting for feedbacks
It is time to move forward on this issue after long discussions.

….hide')

If they checked by default - that is breakable change

facebook-github-bot · 2018-10-15T19:40:12Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot · 2018-10-15T19:49:17Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Gvozd · 2018-10-18T20:28:15Z

Call in the discussion of interested persons
@gaearon @sebmarkbage @Kovensky @zackargyle @kyleshevlin @jquense @jaredly @shriah @bvaughn

jaredly · 2018-10-19T02:08:11Z

Looks good to me! I'm no longer a committer on this project, so I'll have to let someone else merge

bvaughn · 2018-10-19T11:42:58Z

I'm not sure how I feel about the hidden symbol. I don't like the hideDisplayNamed logic on initial read though. It seems unintuitive to me. Maybe you can explain your thought process?

bvaughn · 2018-10-19T11:37:37Z

backend/attachRendererFiber.js

@@ -176,6 +178,8 @@ function attachRendererFiber(hook: Hook, rid: string, renderer: ReactRenderer):
        publicInstance = fiber.stateNode;
        props = fiber.memoizedProps;
        state = fiber.memoizedState;
+        hideSymbol = typeof Symbol === 'function' && fiber.type[Symbol.for('react.devtools.hide')];
+        hideDisplayNamed = fiber.type.hasOwnProperty('displayName');


Sorry, I'm jet lagged at the moment so maybe this is a stupid question, but why is the "displayName" check here? I don't get how it's related.

bvaughn · 2018-10-19T11:38:39Z

frontend/Breadcrumb.js

@@ -163,7 +163,7 @@ function getBreadcrumbPath(store: Store): BreadcrumbPath {
 }

 module.exports = decorate({
-  listeners: () => ['breadcrumbHead', 'selected'],
+  listeners: () => ['breadcrumbHead', 'selected', 'hideSymbol', 'hideDisplayNamed'],


Why is Breadcrumb listening for hideDisplayNamed and hideSymbol when it doesn't use them?

It's used in getBreadcrumbPath > store.skipWrapper

bvaughn · 2018-10-19T11:40:58Z

frontend/PreferencesPanel.js

+                checked={hideDisplayNamed}
+                onChange={this._changeHideDisplayNamed}
+              />
+              Hide components with custom <code>displayName</code> property


I don't understand why you'd want this particular functionality.

Many HOC's creates wrapper components with custom displayName like SomeWrapper(OriginalComponent)
In contrast, regular components usually do not use dispalyName

I thought that such a simple rule is suitable for a draft, and better than nothing
This is the minimum that I would like to get in order to hide noisy HOC's
I can add/replace with other simple HOC recognition, if they work better.

Many HOC's creates wrapper components with custom displayName
In contrast, regular components usually do not use dispalyName

There are many that are kind of like "presentation HOC" and assign displayName through babel plugin, i.e styled-components, emotion, jss, etc:

const MyButton = styled.button` color: tomato; ` // MyButton will have displayName: "MyButton" crafted by babel plugin

Although they are technically HOC i'd not want to hide them.

I think the displayName attribute is too likely to cause false positives, as @asvetliakov points out, due to popular plug-ins like babel-plugin-transform-react-display-name. It also feels too arbitrary to me.

bvaughn · 2018-10-19T11:42:55Z

backend/attachRendererFiber.js

@@ -176,6 +178,8 @@ function attachRendererFiber(hook: Hook, rid: string, renderer: ReactRenderer):
        publicInstance = fiber.stateNode;
        props = fiber.memoizedProps;
        state = fiber.memoizedState;
+        hideSymbol = typeof Symbol === 'function' && fiber.type[Symbol.for('react.devtools.hide')];


nit: react-devtools.hide?

Inve1951 · 2018-10-19T16:25:45Z

I find the method discussed in #676 more appealing.

How about we just dim the components in the tree that have ( or ) in their names? These are always HOCs, as people commonly use X(Y) convention like Connect(App), Relay(FeedItem), or withStyles(Button). You can’t accidentally put parens into the name so it’s always intentional. HOCs that don’t adhere to this convention could easily adapt to it.

For hiding that is, not dimming. Could also have a checkbox in devtools to enable/disable the feature (or maybe even toggle between hiding and dimming).

bvaughn · 2018-10-19T19:43:04Z

I believe I would also be more of a fan of that approach, as it would be more consistent with filtering other categories of components (as described in #1076)

…ble Node

- correct merge with default value from Store - need call handler for changes emited at another handler

…vtools.hide # Conflicts: # backend/attachRendererFiber.js

danielkcz · 2018-10-25T18:59:24Z

This was so good looking until it wasn't. Would you please remove the second approach with displayName? It's too much opinionated and not that useful. Even though it's opt-in, it would be just confusing.

Gvozd · 2018-10-25T19:57:41Z

Sorry for the break
Replaced 'Hide components with custom 'displayName' property to Hide HOC's with parens in name
Fixed some bug with breadcrumbs, and selection
Corrected UI

In next plans:

additional dimming mode
optimize performance

danielkcz · 2018-10-25T22:08:38Z

@Gvozd thanks, much better.

I am wondering, won't this symbol approach be somewhat problematic in TypeScript or possibly even Flow? Generally attaching foreign properties to objects is rather painful with static typing. Any ideas how it could be solved except using // @ts-ignore above the assignment line?

Jessidhia · 2018-10-26T02:43:40Z

I wasn't thinking of attaching it to arbitrary functions but rather to functions/classes you generate yourself as part of the HOC factory function. static [Symbol.for('react-devtools.hide')] = true.

danielkcz · 2018-10-26T06:48:01Z

Well, for an example I would like to hide all React.createContext so I suppose I would have to do something like this?

const context = React.createContext()
// @ts-ignore
context.Consumer[hideSymbol] = true

This is especially troubling for 3rd party ones. And also some stateless components are useless to be visible in the tree, so that's about attaching a symbol to the existing function.

use-case: - one of hiding checkbox is enabled - open full tree of HigherOrderComponent(Nested) - toggle another checkbox. TreeView is not changed(because HOC hided by another checkbox) Changed timings for `wrapMultipleNested(Nested, 25/50/100)` ms: 413/1170/4044 -> 54/94/184 O: O(n^2) -> O(n)

Gvozd · 2018-10-27T23:18:33Z

I fixed the performance of many nested components
For custom static symbol/ordinary property
If they are approved by maintainers - need fix ts/flow lib definition for react
In this case, setting this property should work as well as displayName
It is also necessary to use an ordinary property, in order to properly support the flow
Offer hideInDevtools as the name of a static property

danielkcz · 2018-10-28T07:24:41Z

I am thinking this is not needed as much anymore by introducing React Hooks. It's obvious that need for HOC and various middle-man components will be gone with this and we will have a naturally clean component tree 🎉

Gvozd · 2018-10-28T22:08:15Z

TLDR: There are no plans to remove classes from React.

The old HOCs will not disappear anywhere for a long time
Classes do not go anywhere, so the hooks will live for a long time in parallel with the existing APIs, classes, and therefore HOC/render props

It also takes time

stabilize RFC
while the most popular libraries will add hoke API
In addition, because hooks do not work with class components, and no one is going to refuse classes
when (if) existing projects will be transferred to hooks, and exclusively functional components

This is the case if all developers accept hooks as more convenient, and decide to switch to them in full
Until then (an optimistic assessment for the next year or two), the HOC will be a frequent guest in many already existing projects

So I think that to hide the HOCis still necessary and useful.

…emStyle

Gvozd · 2018-11-01T06:59:45Z

Upd: support dimming

…vtools.hide # Conflicts: # frontend/Store.js

Gvozd · 2018-11-12T18:02:01Z

I think I need to leave only one option of hiding/dimming, for the HOC with parentheses in the name
It is also worth moving these options to SettingsPane, as in #503, only in the form of selectbox, so as not to take up much space
What you think?

Inve1951 · 2018-11-16T21:11:52Z

I think I need to leave only one option of hiding/dimming, for the HOC with parentheses in the name

I think dimming should be the default. A toggle to hide instead would be usefull.
So basically, off = dimmed, on = hidden.

It is also worth moving these options to SettingsPane, as in #503, only in the form of selectbox, so as not to take up much space

Good idea.

bvaughn · 2018-11-19T23:15:31Z

I don't think the dimming is very useful. It's very subtle and the tree is just as cluttered as before. (I think the primary purpose of hiding components is that the element tree is too crowded.)

I also think it's confusing to have a checkbox that says, e.g. "Hide HOCs..." when they won't actually be hidden unless you select one of three radio options above. I suppose I could imagine a UX where we have a global toggle on/off, but I don't think it's necessary.

I realize this PR has been pretty long running, and I feel bad about the inconvenience. If you'd like, I could pick up where this PR leaves off and make the additions I had planned for #1076 originally?

Gvozd · 2018-11-19T23:32:12Z

@bvaughn, OK.

Inve1951 · 2018-11-20T06:01:11Z

I don't think the dimming is very useful. It's very subtle and the tree is just as cluttered as before. (I think the primary purpose of hiding components is that the element tree is too crowded.)

I personally appreciate a dimming option for HOCs, as they are part of the virtual DOM and I'd like to view them as such, but at the same time want them to be visually distinguishable/faded since they are HOCs / less impactful than regular components.

I also think it's confusing to have a checkbox that says, e.g. "Hide HOCs..." when they won't actually be hidden unless you select one of three radio options above. I suppose I could imagine a UX where we have a global toggle on/off, but I don't think it's necessary.

We just talked about this and I agree that the radio buttons are inconvenient. My suggestion was to make a checkbox/toggle for hiding. If dimming gets included in this feature then it could be the off-state of said toggle.

@bvaughn @Gvozd

Diogo Franco (Kovensky) and others added 7 commits October 15, 2018 22:29

Support hiding components in devtools with Symbol.for('react.devtools…

3a11172

….hide')

Correctly listen to hideSymbol updates in Breadcrumb

223b073

Fix flow error, style

b5bbe03

Clean after rebase

6eff343

Option for hide components with displayName(usually HOC)

0bdd7c9

Default disable hide options

a132e4d

If they checked by default - that is breakable change

Remove unnecessary code

b7a6e15

Gvozd mentioned this pull request Oct 15, 2018

Support hiding components in devtools with Symbol.for('react.devtools.hide') #997

Closed

facebook-github-bot added the CLA Signed label Oct 15, 2018

Save hide-options to persistent storage

bc2151b

bvaughn reviewed Oct 19, 2018

View reviewed changes

Gvozd added 5 commits October 23, 2018 01:01

Refactoring: extract method for hidden nodes

f52b355

Bugfix: correct selection and collapsing at hide HOC's - nearest visi…

941e900

…ble Node

Refactoring/bugfix: work with setted default value of checkbox

3dd3946

- correct merge with default value from Store - need call handler for changes emited at another handler

Refactoring: re-use SettingsCheckbox for new options

62a876a

Merge remote-tracking branch 'remotes/upstream/master' into _react.de…

725ae2d

…vtools.hide # Conflicts: # backend/attachRendererFiber.js

Replace 'custom displayName' preference to 'with parens in name'

45d1939

Gvozd added 2 commits November 1, 2018 09:27

Preparatory refactoring: possible additional args for jsxTagStyle, it…

e6024b5

…emStyle

Support dimming nodes

6e704e9

Gvozd added 2 commits November 1, 2018 11:32

bug fix: no highlight on open preferences panel

cd428a7

Merge remote-tracking branch 'remotes/upstream/master' into _react.de…

c0eb886

…vtools.hide # Conflicts: # frontend/Store.js

Gvozd changed the title ~~Support hiding components in devtools~~ [WIP] Support hiding components in devtools Nov 12, 2018

Gvozd closed this Dec 13, 2018

[WIP] Support hiding components in devtools #1187

[WIP] Support hiding components in devtools #1187

Uh oh!

Conversation

Gvozd commented Oct 15, 2018

Uh oh!

facebook-github-bot commented Oct 15, 2018

Uh oh!

facebook-github-bot commented Oct 15, 2018

Uh oh!

Gvozd commented Oct 18, 2018

Uh oh!

jaredly commented Oct 19, 2018

Uh oh!

bvaughn commented Oct 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Inve1951 commented Oct 19, 2018

Uh oh!

bvaughn commented Oct 19, 2018

Uh oh!

danielkcz commented Oct 25, 2018

Uh oh!

Gvozd commented Oct 25, 2018

Uh oh!

danielkcz commented Oct 25, 2018

Uh oh!

Jessidhia commented Oct 26, 2018

Uh oh!

danielkcz commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gvozd commented Oct 27, 2018

Uh oh!

danielkcz commented Oct 28, 2018

Uh oh!

Gvozd commented Oct 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gvozd commented Nov 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gvozd commented Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Inve1951 commented Nov 16, 2018

Uh oh!

bvaughn commented Nov 19, 2018

Uh oh!

Gvozd commented Nov 19, 2018

Uh oh!

Inve1951 commented Nov 20, 2018

Uh oh!

Uh oh!

danielkcz commented Oct 26, 2018 •

edited

Loading

Gvozd commented Oct 28, 2018 •

edited

Loading

Gvozd commented Nov 1, 2018 •

edited

Loading

Gvozd commented Nov 12, 2018 •

edited

Loading