-
Notifications
You must be signed in to change notification settings - Fork 47.4k
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
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 |
@@ -96,7 +97,9 @@ class ReactShallowRenderer { | |||
if (this._instance) { | |||
this._updateClassComponent(element, this._context); | |||
} else { | |||
if (shouldConstruct(element.type)) { | |||
if (isForwardRef(element)) { | |||
this._rendered = element.type.render(element.props, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
@@ -34,7 +34,7 @@ export function typeOf(object: any) { | |||
case REACT_STRICT_MODE_TYPE: | |||
return type; | |||
default: | |||
const $$typeofType = type.$$typeof; | |||
const $$typeofType = type && type.$$typeof; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's possible to have a null React element type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not in practice but the ShallowRenderer tests do <NullIdentifer>
which was how i was hitting this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. 👍
@@ -17,16 +13,11 @@ | |||
"dependencies": { | |||
"fbjs": "^0.8.16", | |||
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These versions are bumped by our release script manually. It's fine. I'll add it to my list of follow up things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed via 610df95
<span className="child1" />, | ||
<span className="child2" />, | ||
]); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" />,
]);
});
@@ -96,7 +97,9 @@ class ReactShallowRenderer { | |||
if (this._instance) { | |||
this._updateClassComponent(element, this._context); | |||
} else { | |||
if (shouldConstruct(element.type)) { | |||
if (isForwardRef(element)) { | |||
this._rendered = element.type.render(element.props, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.