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

Added support for React.memo #1914

Open
wants to merge 1 commit into
base: master
from

Conversation

10 participants
@jintoppy
Copy link

jintoppy commented Nov 22, 2018

This is based on and fixes #1875.

@jintoppy jintoppy referenced this pull request Nov 22, 2018

Open

New `memo` feature from React 16.6 is not supported #1875

3 of 13 tasks complete
@alexgurr

This comment has been minimized.

Copy link

alexgurr commented Nov 22, 2018

When I used the code directly from #1875 (comment) (that you have ported to enzyme) it works, but only for top level memo'd components. If there are nested components that are wrapped in memo(), they still display in the DOM as [object Object]. Does this code address that?

I've had to mock every "sub" memo wrapped component using Jest, otherwise the component isn't searchable by name in the DOM.

@ljharb
Copy link
Member

ljharb left a comment

Thanks, great start!

Can we make the shallow and mount tests as close to identical as possible?

Let's also add some debug tests for this.

Show resolved Hide resolved packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js Outdated
@@ -513,6 +527,7 @@ class ReactSixteenAdapter extends EnzymeAdapter {
switch ($$typeofType) {
case ContextConsumer || NaN: return 'ContextConsumer';
case ContextProvider || NaN: return 'ContextProvider';
case Symbol.for('react.memo') || NaN: return 'Memo';

This comment has been minimized.

@ljharb

ljharb Nov 23, 2018

Member
Suggested change Beta
case Symbol.for('react.memo') || NaN: return 'Memo';
case REACT_MEMO_TYPE || NaN: return 'Memo';
Show resolved Hide resolved packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx Outdated
Show resolved Hide resolved packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx Outdated
Show resolved Hide resolved packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx Outdated
Show resolved Hide resolved packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx Outdated
@jintoppy

This comment has been minimized.

Copy link
Author

jintoppy commented Nov 23, 2018

@alexgurr Yes. It handles nested memoized components as well.

@jintoppy

This comment has been minimized.

Copy link
Author

jintoppy commented Nov 23, 2018

@ljharb Thanks so much for the quick response and taking time to review. I have made the changes.
REACT_MEMO_TYPE was coming as undefined. So, I added a null check also. Not sure if I am doing that in the right way. Please check and let me know

Show resolved Hide resolved packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js Outdated
@@ -249,6 +251,10 @@ function getEmptyStateValue() {
return testRenderer._instance.state;
}

function isMemo(node) {
return typeof node.type === 'object' && node.type.$$typeof === REACT_MEMO_TYPE_SYMBOL;

This comment has been minimized.

@ljharb

ljharb Nov 26, 2018

Member
Suggested change Beta
return typeof node.type === 'object' && node.type.$$typeof === REACT_MEMO_TYPE_SYMBOL;
return typeof node.type === 'object' && REACT_MEMO_TYPE && node.type.$$typeof === REACT_MEMO_TYPE;
const wrapper = mount(<Foo foo="qux" />);
expect(wrapper.debug()).to.equal(`<Component foo="qux">
<div>
<InnerComp>

This comment has been minimized.

@ljharb

ljharb Nov 26, 2018

Member

hmm - i'd expect some sort of indication here that it's memoized.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Nov 26, 2018

ahhhhh i see that REACT_MEMO_TYPE isn't even exported :-/ https://unpkg.com/react-is@16.6.3/cjs/react-is.development.js

That means we'll have to temporarily hardcode it with var hasSymbol = typeof Symbol === 'function' && Symbol.for; var REACT_MEMO_TYPE = hasSymbol ? Symbol.for('react.memo') : 0xead3;, which is a shame.

@jintoppy

This comment has been minimized.

Copy link
Author

jintoppy commented Nov 27, 2018

@ljharb I will then wait for the PR in react to be merged, and will update this PR.

@koragan

This comment has been minimized.

@jintoppy

This comment has been minimized.

Copy link
Author

jintoppy commented Nov 29, 2018

@koragan: Sure. will update it soon

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Nov 29, 2018

@koragan the PR is merged, but react-is still hasn't been updated.

@phenax

This comment has been minimized.

Copy link

phenax commented Dec 3, 2018

When can we expect this patch to be published? A few of the production project that we're working on depend on it.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Dec 3, 2018

@phenax first we have to wait until react-is v16 gets updated.

@amite

This comment has been minimized.

Copy link

amite commented Dec 3, 2018

react-is seems to have support for React Memo: https://github.com/facebook/react/blob/master/packages/react-is/src/ReactIs.js#L120

What exactly is pending? 🤔

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Dec 3, 2018

@amite yes, it's been merged, but not yet released to npm by the react team.

@rpearce

This comment was marked as resolved.

Copy link

rpearce commented Dec 5, 2018

Update: latest react-is version 16.6.3 was published to npm 2 days ago

@jintoppy

This comment was marked as resolved.

Copy link
Author

jintoppy commented Dec 5, 2018

the version in npm is still 16.6.3 and it is released 22 days ago. isMemo check got added after that only. if you install the latest react-is package, you can see that, isMemo check is not available.

https://unpkg.com/react-is@16.6.3/cjs/react-is.development.js

@rpearce

This comment was marked as resolved.

Copy link

rpearce commented Dec 6, 2018

@jintoppy My mistake! Thank you for the correction

@mizchi mizchi referenced this pull request Dec 6, 2018

Merged

Add more type annotation #256

@James300 James300 referenced this pull request Dec 10, 2018

Open

WIP: Tabs #422

0 of 2 tasks complete
@MarceloAlves

This comment has been minimized.

Copy link

MarceloAlves commented Dec 20, 2018

react-is 16.7.0 was finally released 🎉

@ljharb ljharb force-pushed the jintoppy:master branch from f9afd24 to f01c495 Dec 20, 2018

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Dec 21, 2018

I've updated react-is in master and rebased this branch. I'm not sure what's up with the test failures; it might be an issue in enzyme's overall test setup. I'll look into it.

@ljharb ljharb force-pushed the jintoppy:master branch 3 times, most recently from ba622ce to bcbb1e8 Dec 21, 2018

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Dec 22, 2018

The underlying problem has been fixed and this branch rebased.

@ljharb
Copy link
Member

ljharb left a comment

Turns out this still needs some work; React itself seems to not render an additional component in the tree for memoized components, so you can't actually .find by a memoized instance directly (unless we unwrap the memoized constructor and search instead for the inner one, and do the reverse in shallow).

I'll try to see what I can do.

@copiali

This comment has been minimized.

Copy link

copiali commented Jan 14, 2019

Any update?

@ljharb ljharb added this to v16.6: memo, lazy, suspense in React 16 Jan 23, 2019

@ljharb ljharb force-pushed the jintoppy:master branch from bcbb1e8 to 2dbfc79 Feb 7, 2019

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Feb 7, 2019

No update yet. I've rebased this; I'll give it another pass tomorrow.

@RoystonS

This comment has been minimized.

Copy link

RoystonS commented Feb 7, 2019

Btw, there appear to be two React memo tags, not just one: MemoComponent (currently tag 14) and SimpleMemoComponent (currently tag 15). I just got bitten by this on our in-house custom-patched enzyme where we were only coping with tag 15, and I suddenly hit a 14 tag.

I'm not at all sure of the exact details as I only got hit by this a few minutes ago, but it looks like React.memo(fn) gets you a simple component (tag 15) and React.memo(class) gets you a non-simple component (tag 14).

I think the PR at present would only cope with tag 15.

@RoystonS

This comment has been minimized.

Copy link

RoystonS commented Feb 7, 2019

I've done a bit more digging, and it is the case that there's one tag type for React.memo(FunctionalComponent) and a different tag type for React.memo(ClassComponent)

Details: https://codesandbox.io/s/kwvx4qwxyo (tags are output to the console)

@ljharb

ljharb approved these changes Feb 9, 2019

@ljharb ljharb force-pushed the jintoppy:master branch 2 times, most recently from b5dee45 to 5649868 Feb 9, 2019

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Feb 9, 2019

I've rebased and updated this to have tests for both class-based and SFC components.

facebook/react#14807 may be a blocker.

@RoystonS

This comment has been minimized.

Copy link

RoystonS commented Feb 9, 2019

Btw, React are trying to move away from the term 'SFC':
https://twitter.com/dan_abramov/status/1057625147216220162

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Feb 10, 2019

Per facebook/react#14807 (comment), the shallow renderer will need to be updated before this can go in as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment