Skip to content

Conversation

bvaughn
Copy link
Owner

@bvaughn bvaughn commented May 24, 2019

This way we can avoid unnecessarily re-rendering function components with hooks (which can be annoying if there are breakpoints or console logs in those functions).

This is meant to help with issues like facebook/react-devtools#1215 (comment) and hopefully also (to a lesser extent) #195 .

@bvaughn bvaughn force-pushed the cache-inspected-element branch from e0ed2ee to efc2f82 Compare May 24, 2019 16:17
@bvaughn bvaughn merged commit 4cc7e74 into master May 24, 2019
@bvaughn bvaughn deleted the cache-inspected-element branch May 24, 2019 16:26
return id;
}

mostRecentlyInspectedElementID = id;
Copy link
Contributor

Choose a reason for hiding this comment

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

This mostRecentlyInspectedElementID assumes that there's only one most recently inspected element, but (in my experience) during most debugging sessions more than one element gets inspected, the person who does the debugging jumps from element to element to find the issue, so this optimization, although helpful when sitting on one element, is invalidated for the usual debugging sessions. Of course I've got no data to prove that.

Copy link
Owner Author

@bvaughn bvaughn May 28, 2019

Choose a reason for hiding this comment

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

I don't think I agree with the idea that it's "invalid" for a typical debugging session. The problem this PR was addressing was that we polled a component once a second, which felt like an infinite loop.

We could cache data for the most recent N elements, although then our invalidation check would become more expensive. I suspect this fix is sufficient but we can revisit if not.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess "invalidated" was, again, a too-strong word (but I don't know other words for this thought, recommendations?).

It's great that it solves/replaces polling, so please do not consider my comments as de-valueing the work done here.

As we do not have real data, we might only guess how this will perform. I had no chance to test this yet in the same scenario where I struggled with breakpoints hit by polling (not every day gives problems that require such debugging), so I expressed general concerns about the approach.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Cool, cool. Let's talk again when you've had a chance to evaluate the change in the context of debugging with a breakpoint, etc. (also keeping in mind that #15726 may improve the experience further).

I think this change is not sufficient by itself, but it does get us incrementally closer.

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

Successfully merging this pull request may close these issues.

2 participants