Support ForwardRef type of work in TestRenderer#12392
Conversation
|
Not sure how to update ShallowRenderer either |
|
i don't understand why the react CI hates me so much |
| } else { | ||
| if (shouldConstruct(element.type)) { | ||
| if (isForwardRef(element)) { | ||
| this._rendered = element.type.render(element.props, null); |
There was a problem hiding this comment.
This seems fine in the case where you're rendering just host elements, but maybe not helpful for the HOC case where you wrap and render a single composite component...
There was a problem hiding this comment.
I think this line should be:
this._rendered = element.type.render(element.props, element.ref);I'm not sure I understand your PR comment though?
There was a problem hiding this comment.
I was thinking of this case:
function withFoo(Component) {
const Foo = (props) => <Component ref={props.forwardedRef} />
return React.forwardRef((props, ref) => <Foo forwardedRef={ref} />)
}
shallow(<WithFooComponent><div/></WithFooComponent>)You'd get a different result than without the forwardRef but I guess that's true of any HoC thing, might even help hide the HoC guts a bit more with is nice.
There was a problem hiding this comment.
I see. In general, I'm not sure how valuable shallow testing with anything refs or HOC related is. 😄
|
CI is down right now for ReactJS.org repo too:
Looks like this is being caused by an upstream Docker issue, nodejs/docker-node/issues/651 |
| return type; | ||
| default: | ||
| const $$typeofType = type.$$typeof; | ||
| const $$typeofType = type && type.$$typeof; |
There was a problem hiding this comment.
I don't think it's possible to have a null React element type?
There was a problem hiding this comment.
probably not in practice but the ShallowRenderer tests do <NullIdentifer> which was how i was hitting this.
| "object-assign": "^4.1.1", | ||
| "prop-types": "^15.6.0" | ||
| "prop-types": "^15.6.0", | ||
| "react-is": "^16.3.0-alpha.2" |
There was a problem hiding this comment.
Will this require updates to our release/build script to keep these deps in sync? Needs follow up.
There was a problem hiding this comment.
hmm i don't know, I was conflating workspaces and lerna, the latter of which handles the updates
There was a problem hiding this comment.
These versions are bumped by our release script manually. It's fine. I'll add it to my list of follow up things.
| <span className="child1" />, | ||
| <span className="child2" />, | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
Suggestion:
it('should handle ForwardRef', () => {
const testRef = React.createRef();
const SomeComponent = React.forwardRef((props, ref) => {
expect(ref).toEqual(testRef);
return (
<div>
<span className="child1" />
<span className="child2" />
</div>
)
});
const shallowRenderer = createRenderer();
const result = shallowRenderer.render(<SomeComponent ref={testRef} />);
expect(result.type).toBe('div');
expect(result.props.children).toEqual([
<span className="child1" />,
<span className="child2" />,
]);
});| } else { | ||
| if (shouldConstruct(element.type)) { | ||
| if (isForwardRef(element)) { | ||
| this._rendered = element.type.render(element.props, null); |
There was a problem hiding this comment.
I think this line should be:
this._rendered = element.type.render(element.props, element.ref);I'm not sure I understand your PR comment though?
|
Thanks for this PR, by the way! 😁 |
|
Lint, Flow, and tests pass for me locally so I'm going to move forward in spite of the CI/Docker issue. |
* Support ForwardRef type of work in TestRenderer and ShallowRenderer. * Release script now updates inter-package dependencies too (e.g. react-test-renderer depends on react-is).
* Support ForwardRef type of work in TestRenderer and ShallowRenderer. * Release script now updates inter-package dependencies too (e.g. react-test-renderer depends on react-is).
* Support ForwardRef type of work in TestRenderer and ShallowRenderer. * Release script now updates inter-package dependencies too (e.g. react-test-renderer depends on react-is).
No description provided.