Expose injectIntoDevTools() to renderers #11463
Conversation
|
@iamdustan Can you verify this solves your use case? |
| } | ||
| return findFiberByHostInstance(instance); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Are the wrapper functions necessary? Couldn't we just do...
injectIntoDevTools(devToolsConfig: DevToolsConfig<I, TI>): boolean {
const {findFiberByHostInstance} = devToolsConfig;
const findFiberByHostInstanceImpl = findFiberByHostInstance
? findFiberByHostInstance
: () => null;
return ReactFiberDevToolsHook.injectInternals({
...devToolsConfig,
findHostInstanceByFiber: findHostInstance,
findFiberByHostInstance: findFiberByHostInstanceImpl,
});
},There was a problem hiding this comment.
That would work too. I don't feel strongly as they're only called once.
There was a problem hiding this comment.
I tried using your approach, but it seems like Flow fails to catch some issues for some reason now. I'd prefer keeping it explicit so it's better typed.
There was a problem hiding this comment.
Boo, Flow.
Okay. No biggie.
There was a problem hiding this comment.
I missed the merge by a few minutes, but copying this build into Yomguithereal/react-blessed#60 and updating accordingly didn’t seem to work out of the box like before. I think that we need the FiberTreeReflection exposed as well 😬
| findFiberByHostInstance?: (instance: I | TI) => Fiber, | ||
| // Used by RN in-app inspector. | ||
| // This API is unfortunately RN-specific. | ||
| // TODO: Change it to accept Fiber instead and type it properly. |
| // Used by RN in-app inspector. | ||
| // This API is unfortunately RN-specific. | ||
| // TODO: Change it to accept Fiber instead and type it properly. | ||
| getInspectorDataForViewTag?: (tag: number) => Object, |
There was a problem hiding this comment.
does the isAppActive() method no longer exist?
There was a problem hiding this comment.
Can you remind me where it is used?
There was a problem hiding this comment.
It used to be part of the devtools config I think. https://github.com/facebook/react-devtools/blob/ef493abba0d08c0243bdb1f2183edc797d5d366c/packages/react-devtools-core/src/backend.js#L13-L18
I think the usage was so that an app could indicate whether it should take over the devtools connection or not, but not really sure. Or maybe to return false while still starting up (according to this comment)
There was a problem hiding this comment.
It's not required. I added it for RN so that multiple apps on simulator don't steal devtools from each other.
There was a problem hiding this comment.
Also, it's part of the options to connectToDevTools() call which sets up a websocket connection.
This is not related to the injectIntoDevTools call which injects renderer into a global variable.
| rendererPackageName: string, | ||
| // Note: this actually *does* depend on Fiber internal fields. | ||
| // Used by "inspect clicked DOM element" in React DevTools. | ||
| findFiberByHostInstance?: (instance: I | TI) => Fiber, |
There was a problem hiding this comment.
I think the devtools integration used to work without this method 🤔
With this being required I think we’d also need ReactFiberTreeReflection.findCurrentHostFiber and ReactFiberTreeReflection.findCurrentHostFiberWithNoPortals also exposed.
There was a problem hiding this comment.
I don't think we need them. This one doesn't have to be the current one. So as long as you save the fiber on some field on the instance (or use a weakmap), you can get one back.
There was a problem hiding this comment.
It still does. The implementation (below) is:
findFiberByHostInstance(instance: I | TI): Fiber | null {
if (!findFiberByHostInstance) {
// Might not be implemented by the renderer.
return null;
}
return findFiberByHostInstance(instance);
},There was a problem hiding this comment.
That's also true. (Though it's nice to implement if you can I guess.)
By “updating accordingly” you mean...? It’s not clear what you added there :-) It won't just work out of the box, you need to call this method. See changes I did in ReactDOM for example. |
|
If you can give me a branch that doesn't work I can take a look. |
|
I just pushed to Yomguithereal/react-blessed#60 with the “update accordingly” bit 😄 |
|
You can test it by copying a fresh build from master in, run |
I think you're passing a config for The config you need to pass to The config you need to pass to |
|
👍 got it! This is working like a charm. Thanks, Dan!
|
|
Sweet. |
An alternative to #11445.
This lets third party renderers inject themselves into the devtools.
Previously, renderers had to reach into reconciler directly to do this.
However, this wouldn't work for third party renderers.
Instead, I put
injectIntoDevTools()directly on theReactFiberReconciler()return value.This way any renderer can choose to call it (or not call it).
Also moves us closer to not using cross-package
/src/imports.