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 context menu #17608

Merged
merged 10 commits into from Dec 18, 2019
Merged

DevTools context menu #17608

merged 10 commits into from Dec 18, 2019

Conversation

@bvaughn
Copy link
Contributor

bvaughn commented Dec 15, 2019

Inspired by built-in browser console options, this PR adds the ability to copy specific props/state/context/hooks values to the clipboard and to store them in a global variables ($reactTemp).

Checklist

  • Add context menu component and hook.
  • Add backend commands for copying variables, storing global values, and inspecting attributes (works similarly to pre-existing pattern for inspecting React components).
  • Wire up context menu and backend commands to enable interacting with selected element props/state/hooks/context.
  • Add unit tests to cover new behavior.
  • Add DevTools props for enabling new behavior per environment. (Disabled for React Native for now.)

New context menu options

Copy and store-as-global demo video

Works for nested values

Demo of inspecting arbitrary props

@bvaughn bvaughn requested review from threepointone, gaearon and trueadm Dec 15, 2019
@codesandbox

This comment has been minimized.

Copy link

codesandbox bot commented Dec 15, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 80931fb:

Sandbox Source
interesting-morse-sm6w3 Configuration
@sizebot

This comment has been minimized.

Copy link

sizebot commented Dec 15, 2019

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 80931fb

@sizebot

This comment has been minimized.

Copy link

sizebot commented Dec 15, 2019

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 80931fb

if (inspectedElement !== null) {
const value = getInObject(inspectedElement, path);

window.$reactTemp = value;

This comment has been minimized.

Copy link
@gaearon

gaearon Dec 15, 2019

Member

Maybe autoincrement like the browser does? So it's easy to save multiple.

This comment has been minimized.

Copy link
@bvaughn

bvaughn Dec 15, 2019

Author Contributor

I considered that but then I wasn't sure there was a compelling use case for saving multiple, and it gets harder to coordinate with multiple roots/renderers. Seems easy enough to re-assign (in console) if you want to save a second and compare.

This comment has been minimized.

Copy link
@bvaughn

bvaughn Dec 15, 2019

Author Contributor

I guess we could pass the auto-incremented value from the frontend to work around the multiple roots difficulty... I'm just still not convinced it's useful 😄 And it could lead to "memory leaks" if you saved a bunch of things, and any had references to a fiber.

This comment has been minimized.

Copy link
@bvaughn

bvaughn Dec 15, 2019

Author Contributor

I guess the memory concern isn't all that big, since this is a debugging use case. I've added the auto-incremented behavior.

This comment has been minimized.

Copy link
@bvaughn

bvaughn Dec 15, 2019

Author Contributor

After some thought, I think storing separate variable can be practically useful, since you could store a value at one point, then store again after an update, then compare the two values in the console to see what changed.

const pageX =
event.clientX || (event.touches && event.touches[0].pageX);
const pageY =
event.clientY || (event.touches && event.touches[0].pageY);

This comment has been minimized.

Copy link
@gaearon

gaearon Dec 15, 2019

Member

This confused me a bit. I was surprised the property is called pageX/Y everywhere else (e.g. in showMenu API and things it calls) because intuitively I expected clientX/Y to be used.

But it seems like we do use clientX/Y first.

What is the case where it doesn't exist? And does pageX/Y give us the same thing in that fallback? What if we already scrolled a bit, aren't they different in their meaning?

This comment has been minimized.

Copy link
@bvaughn

bvaughn Dec 15, 2019

Author Contributor

TBH this is based on my looking at other context menu implementations. 😄 It seems to work from my testing.

This comment has been minimized.

Copy link
@gaearon

gaearon Dec 15, 2019

Member

Which implementations are you referring to? Googling these exact lines didn't give me results.

I'm confused because client* and page* are two different coordinate systems. If a page is scrolled down, page* will include how much you scrolled, but client* wouldn't.

If we use this value to calculate where to position the menu (by comparing to window.innerHeight/Width), it doesn't make sense to me that page* would be accurate. What matters is how far it is in the viewport — not relative to the page. So I would expect to see a bug there when this branch gets hit. When you say it works in testing, do you mean you specifically tested when

  • the view is scrolled down, and
  • we're using touch events

?

Because that's the case where I'd expect a bug.

My suggested change would be to always use client*, e.g. event.clientX || (event.touches && event.touches[0].clientX). If we don't care to test the "touch" code path, it seems to me that we should start with what makes most sense (client* represents client coordinates), and only use page* if we have a good reason for that (my understanding is we don't have one?).

In either case, if normally we use client* (because most people using DevTools don't use touch), it would make sense to me to rename the properties and arguments to be client* instead of page*. They don't have effect on semantics but they are confusing when they don't match what you really get. E.g. I was looking at the measurement implementation and I was confident there was a bug there due to page* usage, but only later I found we're actually passing e.client* there.

This comment has been minimized.

Copy link
@bvaughn

bvaughn Dec 15, 2019

Author Contributor

Googling these exact lines didn't give me results.

Presumably because Prettier formatted these particular lines in a different way.

After thinking and testing a little more, I believe:

  1. It probably doesn't matter which one we use, since the main DevTools window never scrolls, but
  2. If it did scroll, we'd want to use pageY in both places (not just in the touch event) because otherwise our context menu would potentially be positioned too above where the cursor was.

Then again maybe this is just the meds talking 🤪Am I misunderstanding something?

I'm going to pick this back up and re-think it in the morning. 😄 Let's talk more than.

The "contextmenu" event isn't firing for me inside of an extension anyway so I need to dig in more on why this is.

@bvaughn bvaughn removed request for threepointone and trueadm Dec 15, 2019
@bvaughn bvaughn force-pushed the bvaughn:devtools-context-menu branch from 2d5741c to be9046a Dec 17, 2019
Used this mechanism to add a conditional menu option for inspecting the current value (if it's a function)
@bvaughn

This comment has been minimized.

Copy link
Contributor Author

bvaughn commented Dec 18, 2019

Okay folks. This is ready for review


export function safeSerialize(data: any): string {
const cache = new Set();
return JSON.stringify(data, (key, value) => {

This comment has been minimized.

Copy link
@trueadm

trueadm Dec 18, 2019

Member

Can you please explain what makes this safe?

This comment has been minimized.

Copy link
@bvaughn

bvaughn Dec 18, 2019

Author Contributor

Maybe "safe" is a poor choice of words. This function protects against circular references, which would otherwise cause JSON.stringify to throw.

This comment has been minimized.

Copy link
@trueadm

trueadm Dec 18, 2019

Member

Ah okay, I was wondering in cases of using Symbols or functions etc, would it be safe for those too? Naming things is hard :/

This comment has been minimized.

Copy link
@bvaughn

bvaughn Dec 18, 2019

Author Contributor

It would be "safe" in that case, it would just not copy anything meaningful to the clipboard. (If a value can't be serialized, JSON.stringify would just return undefined.)

I renamed "safeSerialize" to "serializeToString" though and added an inline comment about its purpose.

This comment has been minimized.

Copy link
@Fausto95

Fausto95 Dec 18, 2019

That’s a cool trick @bvaughn 👌🏽


useEffect(
() => {
if (ref.current !== null) {

This comment has been minimized.

Copy link
@trueadm

trueadm Dec 18, 2019

Member

Can ref.current change nodes between renders?

This comment has been minimized.

Copy link
@bvaughn

bvaughn Dec 18, 2019

Author Contributor

In our limited use of this hook, it would not be expected to change.

Copy link
Member

trueadm left a comment

I left a few comments, but overall it looks great! :)

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

bvaughn commented Dec 18, 2019

Thanks for the quick feedback! I'll rename the "safe" function as soon as I can think of a better name.

@bvaughn bvaughn merged commit 933f6a0 into facebook:master Dec 18, 2019
21 checks passed
21 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_experimental Your tests passed on CircleCI!
Details
ci/circleci: flow Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: lint_build Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts_experimental Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: sizebot Your tests passed on CircleCI!
Details
ci/circleci: sizebot_experimental Your tests passed on CircleCI!
Details
ci/circleci: test_build Your tests passed on CircleCI!
Details
ci/circleci: test_build_devtools Your tests passed on CircleCI!
Details
ci/circleci: test_build_experimental Your tests passed on CircleCI!
Details
ci/circleci: test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: test_build_prod_experimental Your tests passed on CircleCI!
Details
ci/circleci: test_dom_fixtures Your tests passed on CircleCI!
Details
ci/circleci: test_source Your tests passed on CircleCI!
Details
ci/circleci: test_source_experimental Your tests passed on CircleCI!
Details
ci/circleci: test_source_persistent Your tests passed on CircleCI!
Details
ci/circleci: test_source_prod Your tests passed on CircleCI!
Details
ci/codesandbox Building packages succeeded.
Details
@bvaughn bvaughn deleted the bvaughn:devtools-context-menu branch Dec 18, 2019
@Daniel15

This comment has been minimized.

Copy link
Member

Daniel15 commented Dec 18, 2019

Nice work!! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.