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

Support ForwardRef type of work in TestRenderer #12392

Merged
merged 5 commits into from
Mar 16, 2018

Conversation

jquense
Copy link
Contributor

@jquense jquense commented Mar 16, 2018

No description provided.

@jquense
Copy link
Contributor Author

jquense commented Mar 16, 2018

Not sure how to update ShallowRenderer either

@jquense
Copy link
Contributor Author

jquense commented Mar 16, 2018

i don't understand why the react CI hates me so much

@jquense jquense requested a review from bvaughn March 16, 2018 16:28
@@ -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);
Copy link
Contributor Author

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...

Copy link
Contributor

@bvaughn bvaughn Mar 16, 2018

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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. 😄

@bvaughn
Copy link
Contributor

bvaughn commented Mar 16, 2018

CI is down right now for ReactJS.org repo too:

/bin/bash: yarn: command not found

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;
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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"
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor

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" />,
]);
});
Copy link
Contributor

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);
Copy link
Contributor

@bvaughn bvaughn Mar 16, 2018

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?

@bvaughn
Copy link
Contributor

bvaughn commented Mar 16, 2018

Thanks for this PR, by the way! 😁

@bvaughn
Copy link
Contributor

bvaughn commented Mar 16, 2018

Lint, Flow, and tests pass for me locally so I'm going to move forward in spite of the CI/Docker issue.

@bvaughn bvaughn merged commit e1ff342 into master Mar 16, 2018
@bvaughn bvaughn deleted the update-test-renderer-forward-ref branch March 16, 2018 18:18
LeonYuAng3NT pushed a commit to LeonYuAng3NT/react that referenced this pull request Mar 22, 2018
* 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).
rhagigi pushed a commit to rhagigi/react that referenced this pull request Apr 19, 2018
* 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).
NMinhNguyen referenced this pull request in enzymejs/react-shallow-renderer Jan 29, 2020
* 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants