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 React.memo in ReactShallowRenderer #14816

Merged
merged 6 commits into from
Mar 15, 2019

Conversation

aweary
Copy link
Contributor

@aweary aweary commented Feb 11, 2019

Fixes #14807

  • Adds support for React.memo in React.ShallowRenderer. Instead of using element.type directly it now checks if the element is REACT_MEMO_TYPE and uses the nested type if it is.

  • Validates the propTypes for the inner component

For testing I just coped ReactShallowRenderer-test and wrapped every component in React.memo to validate everything worked. I don't think we should take approach, but it made it easy to find issues 🤷‍♂️maybe we can have a single set of tests that run with and without memo?

@sizebot
Copy link

sizebot commented Feb 11, 2019

Fails
🚫

node` failed.

Log

Error:  { FetchError: invalid json response body at http://react.zpao.com/builds/master/_commits/f0621fe232f31cb0fcd63992c3440ec1b4ce5813/results.json reason: Unexpected token < in JSON at position 0
    at /home/circleci/project/node_modules/node-fetch/lib/body.js:48:31
    at process._tickCallback (internal/process/next_tick.js:68:7)
  name: 'FetchError',
  message:
   'invalid json response body at http://react.zpao.com/builds/master/_commits/f0621fe232f31cb0fcd63992c3440ec1b4ce5813/results.json reason: Unexpected token < in JSON at position 0',
  type: 'invalid-json' }

Generated by 🚫 dangerJS

Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

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

There are some Flow failures that need fixing, but this looks good to me. :)

@gaearon
Copy link
Collaborator

gaearon commented Feb 11, 2019

If we support memo() and forwardRef(), their combination memo(forwardRef()) should also work. Does it?

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

If we're fixing this we'd need a strategy that works with composing them

@aweary
Copy link
Contributor Author

aweary commented Feb 11, 2019

@gaearon I've added support for memo(forwardRef()) and also made sure that forwardRef(memo()) fails as expected.

What do you think of the testing strategy? Should I remove ReactShallowRendererMemo-test and just rely on the memo tests added to ReactShallowRenderer-test? Do you think it's worth running every test with and without memo?

'Warning: forwardRef requires a render function but received ' +
'a `memo` component. Instead of forwardRef(memo(...)), use ' +
'memo(forwardRef(...))',
{withoutStack: true},
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 test is failing saying Expected test not to call console.error(), but I'm calling toWarnDev here so I don't think that's right?

const shallowRenderer = createRenderer();
shallowRenderer.render(<SomeComponent foo={1} />);
expect(areEqual).not.toHaveBeenCalled();
expect(shallowRenderer.getRenderOutput()).toEqual(<div>1</div>);
Copy link
Contributor Author

@aweary aweary Feb 11, 2019

Choose a reason for hiding this comment

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

This test is failing here because the rendered output doesn't match. I verified that getRenderOutput is returning <div>1</div>. I guess its failing because they're not the same React elements, but we're using this same pattern elsewhere in this test suite so I don't know why its failing here.

@@ -500,7 +500,8 @@ class ReactShallowRenderer {
element.type,
);
invariant(
isForwardRef(element) || typeof element.type === 'function',
isForwardRef(element) ||
(typeof element.type === 'function' || isMemo(element.type)),
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason not to use isValidElementType from react-is here, and asserting the type isn’t string, rather than repeating some of the logic?

Copy link
Contributor Author

@aweary aweary Feb 12, 2019

Choose a reason for hiding this comment

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

Probably because isValidElementType was introduced after this version of the shallow renderer. That's a good point though, because looking at isValidElementType catches more uncommon cases that React technically supports and that fail with the shallow renderer.

For example, you can do:

const MemoFragment = React.memo(React.Fragment)

But rendering that with the shallow render will trigger this invariant. The same goes for other elements that have their own type like React.Suspense, React.ConcurrentMode, React.StrictMode, etc.

@@ -545,14 +565,13 @@ class ReactShallowRenderer {
}
}

if (element.type.hasOwnProperty('contextTypes')) {
if (elementType.hasOwnProperty('contextTypes')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you may want to use Object.prototype.hasOwnProperty.call here, since it’d otherwise work fine to pass a function component with a null prototype.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This codepath is for classes so should be ok. In fact it should probably use dot access since that's what we do in the real one.

// elementType could still be a ForwardRef if it was
// nested inside Memo.
this._rendered =
elementType.$$typeof === ForwardRef
Copy link
Contributor

Choose a reason for hiding this comment

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

should this use isForwardRef(element) (and cache the isForwardRef status in a var, since it seems to be checked multiple times)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a little confusing, but that doesn't work because isForwardRef expects a React Element for the forwardRef, not the actual object returned from forwardRef itself.

const forwardRef = React.forwardRef(...);
isForwardRef(forwardRef) // false
isForwardRef(React.createElement(forwardRef)) // true

@aweary
Copy link
Contributor Author

aweary commented Feb 12, 2019

@gaearon @trueadm how should the shallow renderer handle rendering the element types exported by React like React.StrictMode, React.ConcurrentMode, etc.

shallowRenderer.render(<React.StrictMode>hmm</React.StrictMode>)

Currently they fail with the error:

Invariant Violation: ReactShallowRenderer render(): Shallow rendering works only
with custom components, but the provided element type was `symbol`.

I think it's correct that it fails (there's no reason to shallow render them directly) but the error about a symbol element type is confusing. Should we report the actual symbol name instead?

the provided element type was `React.StrictMode`.

Should we fail with the same error if you try to render React.memo(React.StrictMode)?

Unrelated to this task, but it looks like we also need to update jest-react or whatever utility is responsible for comparing JSX with expect because it reports these types as UNDEFINED

 Expected value to equal:
      <UNDEFINED>lkdol</UNDEFINED>
    Received:
      <UNDEFINED>lol</UNDEFINED>

@ljharb
Copy link
Contributor

ljharb commented Feb 26, 2019

ping on this; I'm going to update enzyme's react 16 adapter so it can handle memo for SFCs, but without this fix it'll still throw for class-based components. It'd be good to close that gap sooner than later.

@trueadm
Copy link
Contributor

trueadm commented Mar 6, 2019

I think it's correct that it fails (there's no reason to shallow render them directly) but the error about a symbol element type is confusing. Should we report the actual symbol name instead?

I think this is all we should do here. Provide a better error message so they understand that these symbols are not supported.

Brandon Dail and others added 5 commits March 15, 2019 21:33
ReactShallowRenderer uses element.type frequently, but with React.memo
elements the actual type is element.type.type. This updates
ReactShallowRenderer so it uses the correct element type for Memo
components and also validates the inner props for the wrapped
components.
@gaearon gaearon merged commit b283d75 into facebook:master Mar 15, 2019
gaearon pushed a commit to gaearon/react that referenced this pull request Mar 22, 2019
* Support React.memo in ReactShallowRenderer

ReactShallowRenderer uses element.type frequently, but with React.memo
elements the actual type is element.type.type. This updates
ReactShallowRenderer so it uses the correct element type for Memo
components and also validates the inner props for the wrapped
components.

* Allow Rect.memo to prevent re-renders

* Support memo(forwardRef())

* Dont call memo comparison function on initial render

* Fix test

* Small tweaks
@gaearon gaearon mentioned this pull request Mar 22, 2019
This was referenced Oct 26, 2019
NMinhNguyen referenced this pull request in enzymejs/react-shallow-renderer Jan 29, 2020
* Support React.memo in ReactShallowRenderer

ReactShallowRenderer uses element.type frequently, but with React.memo
elements the actual type is element.type.type. This updates
ReactShallowRenderer so it uses the correct element type for Memo
components and also validates the inner props for the wrapped
components.

* Allow Rect.memo to prevent re-renders

* Support memo(forwardRef())

* Dont call memo comparison function on initial render

* Fix test

* Small tweaks
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.

6 participants