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

react-debug-tools accepts currentDispatcher ref as param #14556

Merged

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jan 9, 2019

In support of facebook/react-devtools#1272.

The react-debug-tools package can't import shared internals because it isn't bundled along with the application. Instead it needs to accept those internals from DevTools, which gets them from the renderer (#14550).

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 9, 2019

Because inspectHooksOfFiber reads values from the Fiber, DevTools needs logic to determine which is the current fiber. (It general tries to abstract over this with the concept of an "opaque node" which could either be the current or alternate.)

I believe I can hack this in by comparing data.state to the opaque node's memoizedState, but it's kind of leaky and gross. Maybe we can clean this up later.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Jan 10, 2019

Can we make currentDispatcher an optional last argument which defaults to looking up in require('react') but allows you to override it from DevTools? That way anyone using this tool bundled with their app such as if you install it from npm, it just works out of the box without having to drill into the secret object? But you still have the option to.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 10, 2019

Yes, I can make this change. It seems a bit weird to me though because if you forget to inject a value in the DevTools use case, you'll be reading/writing from the wrong dispatcher ref. I see the usefulness for this though in cases like you describe.

Edit Although I guess in this case, React would throw an error right away so at least it wouldn't be a silent failure. Should we catch and re-throw a more meaningful error in this case?

@bvaughn bvaughn force-pushed the debug-hooks-accept-injected-internals branch from ce0f030 to 721f16c Compare January 10, 2019 16:43
@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 10, 2019

Okay. Back to you @sebmarkbage

@sizebot
Copy link

sizebot commented Jan 10, 2019

Details of bundled changes.

Comparing: b4ad8e9...721f16c

react-debug-tools

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-debug-tools.development.js +3.0% +1.8% 16.06 KB 16.53 KB 4.82 KB 4.91 KB NODE_DEV
react-debug-tools.production.min.js 🔺+1.3% 🔺+1.2% 5.1 KB 5.16 KB 2.12 KB 2.14 KB NODE_PROD

Generated by 🚫 dangerJS

@bvaughn bvaughn merged commit f290138 into facebook:master Jan 10, 2019
@bvaughn bvaughn deleted the debug-hooks-accept-injected-internals branch January 10, 2019 20:56
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
)

* react-debug-tools accepts currentDispatcher ref as param

* ReactDebugHooks injected dispatcher ref is optional
n8schloss pushed a commit to n8schloss/react that referenced this pull request Jan 31, 2019
)

* react-debug-tools accepts currentDispatcher ref as param

* ReactDebugHooks injected dispatcher ref is optional
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants