Added support for React.memo #1914

jintoppy · 2018-11-22T18:52:52Z

This is based on and fixes #1875.

alexgurr · 2018-11-22T23:29:48Z

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

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.

packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js

ljharb · 2018-11-23T02:01:58Z

packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js

@@ -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';


Suggested change

case Symbol.for('react.memo') || NaN: return 'Memo';

case REACT_MEMO_TYPE || NaN: return 'Memo';

packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx

jintoppy · 2018-11-23T03:35:50Z

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

jintoppy · 2018-11-23T03:37:21Z

@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

packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js

ljharb · 2018-11-26T00:03:05Z

packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js

@@ -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;


Suggested change

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;

ljharb · 2018-11-26T00:06:41Z

packages/enzyme-test-suite/test/ReactWrapper-spec.jsx

+      const wrapper = mount(<Foo foo="qux" />);
+      expect(wrapper.debug()).to.equal(`<Component foo="qux">
+  <div>
+    <InnerComp>


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

ljharb · 2018-11-26T00:08:20Z

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 · 2018-11-27T14:06:23Z

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

TaylorClay · 2018-11-29T13:40:21Z

@jintoppy looks like you're clear to update this PR

jintoppy · 2018-11-29T14:25:25Z

@Koragan: Sure. will update it soon

ljharb · 2018-11-29T23:27:37Z

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

phenax · 2018-12-03T02:40:44Z

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

ljharb · 2018-12-03T02:52:15Z

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

amite · 2018-12-03T08:09:12Z

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 · 2018-12-03T08:10:10Z

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

MarceloAlves · 2018-12-20T01:50:29Z

react-is 16.7.0 was finally released 🎉

ljharb · 2018-12-21T05:02:35Z

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 · 2018-12-22T17:50:24Z

The underlying problem has been fixed and this branch rebased.

ljharb

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 · 2019-01-14T00:30:07Z

Any update?

ljharb · 2019-02-07T07:49:37Z

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

RoystonS · 2019-02-07T19:38:05Z

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 · 2019-02-07T20:04:45Z

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 · 2019-02-09T08:45:21Z

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

facebook/react#14807 may be a blocker.

RoystonS · 2019-02-09T10:45:07Z

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

ljharb · 2019-02-10T16:21:22Z

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

VincentLanglet · 2019-02-21T11:01:49Z

Any update ?

If it's not easy / possible to make it works for class component, can not we go incrementally ?

Actually it does not work for both functional component and class component. A first implementation working only for functional component is still better.

ljharb · 2019-02-22T18:56:03Z

@VincentLanglet no, it'd be better to have zero support than partial support, and either way this is blocked on react's shallow renderer change.

mattdarveniza · 2019-02-25T01:45:37Z

Support for the case that 99% of people actually want to use memo for (functional components) seems to be a pretty good reason to merge partial support to me. Usage with classes, while supported by React, isn't actually documented anywhere, and I would say it's an edge case or at least very niche.

The failing test is only related to class components so far as I can tell, so if support for that was moved to a separate PR this would be mergeable, we're not waiting on any dependencies for functional components, right?

ljharb · 2019-02-26T00:44:21Z

OK, that's a reasonable argument. I suppose it'll just "start working" once react-test-renderer fixes facebook/react#14807 (comment), at which point we can update the tests here.

- [new] Added support for React.memo (#1914) - [meta] add "directory" field to package.json - [refactor] use `displayNameOfNode` instead of `function.prototype.name` directly

TymchenkoOleksandr · 2019-03-25T10:54:08Z

packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js

@@ -540,6 +571,7 @@ class ReactSixteenAdapter extends EnzymeAdapter {
    switch ($$typeofType) {
      case ContextConsumer || NaN: return 'ContextConsumer';
      case ContextProvider || NaN: return 'ContextProvider';
+      case Memo || NaN: return displayNameOfNode(type.type);


displayName is broken here!

Can you elaborate? a new issue would be ideal.

always getting displayName "Component" with React.memo(). need to set correct displayName each time after applying React.memo().

@TymchenkoOleksandr would you mind filing a new issue, and fill out the template? that way I'll be able to fix it more quickly.

[enzyme-adapter-react-16] [new] Added support for React.memo

13cff52

jintoppy mentioned this pull request Nov 22, 2018

New memo feature from React 16.6 is not supported #1875

Closed

13 tasks

ljharb requested changes Nov 23, 2018

View reviewed changes

ljharb added semver: minor New stuff. package: react adapter: 16 labels Nov 23, 2018

jintoppy mentioned this pull request Nov 23, 2018

Adding isMemo check to react-is package facebook/react#14313

Merged

ljharb reviewed Nov 26, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

mizchi mentioned this pull request Dec 6, 2018

Add more type annotation recruit-tech/redux-pluto#256

Merged

James300 mentioned this pull request Dec 10, 2018

WIP: Tabs priceline/design-system#422

Closed

2 tasks

ljharb force-pushed the master branch from f9afd24 to f01c495 Compare December 20, 2018 22:24

ljharb force-pushed the master branch 3 times, most recently from ba622ce to bcbb1e8 Compare December 22, 2018 17:49

ljharb requested changes Dec 22, 2018

View reviewed changes

ljharb force-pushed the master branch from bcbb1e8 to 2dbfc79 Compare February 7, 2019 07:48

ljharb approved these changes Feb 9, 2019

View reviewed changes

ljharb force-pushed the master branch 2 times, most recently from b5dee45 to 5649868 Compare February 9, 2019 08:44

ljharb mentioned this pull request Feb 9, 2019

Shallow renderer does not support React.memo facebook/react#14807

Closed

[Tests] temporarily allow class components with memo to fail

6b3274b

ljharb force-pushed the master branch from 5649868 to 017a0ec Compare February 26, 2019 00:43

ljharb mentioned this pull request Feb 26, 2019

Support React.memo in ReactShallowRenderer facebook/react#14816

Merged

ljharb force-pushed the master branch from 017a0ec to 6b3274b Compare February 26, 2019 03:25

ljharb merged commit 6b3274b into enzymejs:master Feb 26, 2019

ljharb mentioned this pull request Feb 26, 2019

memo does not work with class components #2025

Closed

ljharb added a commit that referenced this pull request Feb 26, 2019

[enzyme-adapter-react-16] v1.10.0

cafa8c7

- [new] Added support for React.memo (#1914) - [meta] add "directory" field to package.json - [refactor] use `displayNameOfNode` instead of `function.prototype.name` directly

TymchenkoOleksandr reviewed Mar 25, 2019

View reviewed changes

angorayc mentioned this pull request May 28, 2019

[SIEM] Replace Eui chart with elastic charts for siem kpis elastic/kibana#36660

Merged

1 task

	case Symbol.for('react.memo') \|\| NaN: return 'Memo';
	case REACT_MEMO_TYPE \|\| NaN: return 'Memo';

	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;

Added support for React.memo #1914

Added support for React.memo #1914

Conversation

jintoppy commented Nov 22, 2018 • edited by ljharb Loading

alexgurr commented Nov 22, 2018 • edited Loading

ljharb left a comment

Choose a reason for hiding this comment

ljharb Nov 23, 2018

Choose a reason for hiding this comment

jintoppy commented Nov 23, 2018

jintoppy commented Nov 23, 2018

ljharb Nov 26, 2018

Choose a reason for hiding this comment

ljharb Nov 26, 2018

Choose a reason for hiding this comment

ljharb commented Nov 26, 2018

jintoppy commented Nov 27, 2018

TaylorClay commented Nov 29, 2018

jintoppy commented Nov 29, 2018

ljharb commented Nov 29, 2018

phenax commented Dec 3, 2018

ljharb commented Dec 3, 2018

amite commented Dec 3, 2018

ljharb commented Dec 3, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

MarceloAlves commented Dec 20, 2018

ljharb commented Dec 21, 2018

ljharb commented Dec 22, 2018

ljharb left a comment

Choose a reason for hiding this comment

copiali commented Jan 14, 2019

ljharb commented Feb 7, 2019

RoystonS commented Feb 7, 2019

RoystonS commented Feb 7, 2019

ljharb commented Feb 9, 2019

RoystonS commented Feb 9, 2019

ljharb commented Feb 10, 2019

VincentLanglet commented Feb 21, 2019

ljharb commented Feb 22, 2019

mattdarveniza commented Feb 25, 2019

ljharb commented Feb 26, 2019

TymchenkoOleksandr Mar 25, 2019

Choose a reason for hiding this comment

ljharb Mar 26, 2019

Choose a reason for hiding this comment

TymchenkoOleksandr Mar 26, 2019

Choose a reason for hiding this comment

ljharb Mar 26, 2019

Choose a reason for hiding this comment

jintoppy commented Nov 22, 2018 •

edited by ljharb

Loading

alexgurr commented Nov 22, 2018 •

edited

Loading