Exposes the host container to prepareForCommit and resetAfterCommit #12098
Conversation
|
cc @sebmarkbage |
| @@ -93,8 +93,8 @@ export type HostConfig<T, P, I, TI, HI, PI, C, CC, CX, PL> = { | |||
| ): number, | |||
| cancelDeferredCallback(callbackID: number): void, | |||
|
|
|||
| prepareForCommit(): void, | |||
| resetAfterCommit(): void, | |||
| prepareForCommit(containerInfo: any): void, | |||
There was a problem hiding this comment.
These should have the type C. The return data type needs some new generic type argument.
There was a problem hiding this comment.
Updated the typing (commit data referenced as CD), it seems to pass the flow test. I'd have expected to have to add an any into a generic type instantiation somewhere, but Flow seems able to infer it?
|
It would be good to have a test for this. E.g. the noop renderer could be expanded to use this somehow and use that for a test. https://github.com/facebook/react/blob/master/packages/react-noop-renderer/src/ReactNoop.js#L176 |
|
Added tests. It currently doesn't check that the |
|
You can just add another file that more or less does what you do in your own renderer and the way you want to use it. Do you use the return value for something there? The closer to real-life that we can write the test, the better. |
arcanis
force-pushed
the
resetaftercommit
branch
from
863e296 to
1449847
Compare
|
I've added the tests for the commit data. I don't use it right now, but it will be handy when I will get to implementing selection inside the renderer. I noticed it had been removed in #9816, but running flow doesn't yield any error here. |
| return (savedCommitData = {}); | ||
| }, | ||
| resetAfterCommit: function(hostContext, commitData) { | ||
| expect(commitData).toBe(savedCommitData); |
There was a problem hiding this comment.
Could you try to come up with a test case that's more representative of a real world use case? As suggested in Sebastian's comment. Ideally a person should be able to look at this unit test and understand not just what the method does, but why it exists.
There was a problem hiding this comment.
it will be handy when I will get to implementing selection inside the renderer
This sounds interesting. If you can approximate this use case in the unit test, maybe with some comments to clarify, I'll merge the PR :)
There was a problem hiding this comment.
Since I don't use it in my renderer at the moment I removed the commit data from this PR, I'll open a new one when I'll reach the point where it is needed :)
|
Thanks! |
This commit exposes
containerInfo(which per my experience is the second parameter of therenderfunctions, is that correct?) toprepareForCommitand more importantlyresetAfterCommit.I'm using this change in my custom renderer so that I can trigger a flush of my visual display after the React tree has been reconciled while not needing my visual server to be stored in a global.