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

Should not rely on synchronous return value of renderIntoDocument #6756

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .babelrc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"plugins": [
"fbjs-scripts/babel-6/dev-expression",
"syntax-trailing-function-commas",
"babel-plugin-transform-async-to-generator",
"babel-plugin-transform-object-rest-spread",
"transform-es2015-template-literals",
"transform-es2015-literals",
Expand Down
4 changes: 4 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -101,5 +101,9 @@
"unmockedModulePathPatterns": [
""
]
},
"dependencies": {
"babel-plugin-syntax-async-functions": "^6.8.0",
"babel-plugin-transform-async-to-generator": "^6.8.0"
}
}
28 changes: 26 additions & 2 deletions src/test/ReactTestUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,38 @@ function findAllInRenderedTreeInternal(inst, test) {
* @lends ReactTestUtils
*/
var ReactTestUtils = {
renderIntoDocument: function(instance) {
renderIntoDocument: function(element) {
var div = document.createElement('div');
// 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.
// document.documentElement.appendChild(div);
return ReactDOM.render(instance, div);
return ReactDOM.render(element, div);
},

renderIntoDocumentAsync: function(element) {
invariant(typeof element.ref !== 'string',
'String refs can not be used at the top-level element of a render.'
);
var promise = new Promise(function(fulfill, reject) {
var div = document.createElement('div');
// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebmarkbage @spicyj @gaearon Do you guys want to join the bikeshedding on the function name?

// document.documentElement.appendChild(div);
var oldRef = element.ref;
var newRef = (instance) => {
if (oldRef) {
oldRef(instance);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test ensuring the exact expected behavior around refs is defined. eg, right now string refs are still a thing but you'll get runtime errors if you use them here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String refs can't be used at the top level, so it could never be a string ref, but yeah.

}
fulfill(instance);
};
element = React.cloneElement(element, {ref: newRef});
ReactDOM.render(element, div);
});
return promise;
},

isElement: function(element) {
Expand Down
25 changes: 25 additions & 0 deletions src/test/__tests__/ReactTestUtils-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -535,4 +535,29 @@ describe('ReactTestUtils', function() {
expect(hrs.length).toBe(2);
});

pit('will reject string refs in renderIntoDocumentAsync', async function() {
try {
// String refs are illegal at the top level.
await ReactTestUtils.renderIntoDocumentAsync(<div ref="foo" />);
fail('this code should not be reachable');
} catch (e) {
expect(e.message).toBe('String refs can not be used at the top-level element of a render.');
}
});

pit('will fire a callback ref in renderIntoDocumentAsync', async function() {
var count = 0;
function ref(instance) {
count++;
expect(instance.tagName).toBe('DIV');
}
var instance = await ReactTestUtils.renderIntoDocumentAsync(<div ref={ref} />);
expect(instance.tagName).toBe('DIV');
expect(count).toBe(1);
});

pit('will not die if no ref in renderIntoDocumentAsync', async function() {
var instance = await ReactTestUtils.renderIntoDocumentAsync(<div />);
expect(instance.tagName).toBe('DIV');
});
});