Should not rely on synchronous return value of renderIntoDocument #6756
Conversation
ghost
added
jimfb
force-pushed
the
render-into-document-async
branch
2 times, most recently
from
a26f4e3
to
c83ce57
Compare
|
@jimfb updated the pull request. |
jimfb
force-pushed
the
render-into-document-async
branch
from
c83ce57
to
f424a99
Compare
|
@jimfb updated the pull request. |
|
Overall I think this is an ok thing to do. For the record, I talked to Jim offline about what we think future |
| // None of our tests actually require attaching the container to the | ||
| // DOM, and doing so creates a mess that we rely on test isolation to | ||
| // clean up, so we're going to stop honoring the name of this method | ||
| // (and probably rename it eventually) if no problems arise. |
There was a problem hiding this comment.
I think we can safely break away from the renderIntoDocument misnomer for something new. Might make sense to rename renderIntoDocument (with forwarding) at the same time.
renderDetached? Or if we want a mouthful renderIntoDetachedNode. And then the Promise version… I guess ...Async is ok. I still think of "async" functions as taking a callback but I guess Promises are a thing and that's how async/await works.
There was a problem hiding this comment.
We will also eventually need a way to render into a container asynchronously.
What about ReactTestUtils.renderAsync() which optionally accepts the second argument (container) or renders into a detached node if no such container exists.
renderDetached seems verbose, since 99.99% of the time, you don't care that the node is detached. You just want to render something.
Was just an idea. I truly don't have a strong preference here. Just let me know in a more definitive way when everyone is done bikeshedding and then I'll do the global replace.
There was a problem hiding this comment.
@sebmarkbage @spicyj @gaearon Do you guys want to join the bikeshedding on the function name?
|
Can we split this diff in twain? Use this PR to introduce and test the new API, then a 2nd to use it. Or at the very least 2 commits in this PR. |
jimfb
force-pushed
the
render-into-document-async
branch
from
f424a99
to
48f02ff
Compare
|
@jimfb updated the pull request. |
jimfb
force-pushed
the
render-into-document-async
branch
3 times, most recently
from
d18ef57
to
dca5799
Compare
jimfb
force-pushed
the
render-into-document-async
branch
from
dca5799
to
7c0d854
Compare
|
@jimfb updated the pull request. |
|
Ok, I split the usage into #6785 |
|
I think this is ready to go, modulo bikeshedding on the new function's name. We could also just merge this now and fix the name later... :P. |
|
Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired. Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status. |
|
Per @gaearon's comment #6785 (comment) it appears that this isn't something we'll be doing, so I'm closing this out |
Previously,
renderIntoDocumentrelied on the synchronous return value of render. As per #6397, we want to start moving away from relying on this synchronous return value. This PR takes the first step toward that goal, by providing arenderIntoDocumentAsyncmethod, which makes it easy to render into a document and wait on the return value for the purposes of unit testing. Long term, therenderIntoDocumentAsyncis intended to replacerenderIntoDocumentfor all intents and purposes.The primary motivation for this PR is that it brings our unit tests closer to matching the semantics of an incremental reconciler, thereby allowing us to iterate on our incremental reconciler without failing all the unit tests.
cc @sebmarkbage @spicyj @zpao