Add renderProp to ShallowWrapper #1863

dferber90 · 2018-10-11T20:34:07Z

Adds a renderProp method to ShallowWrappers to ease testing render props.

This PR basically extracts renderProp out of @commercetools/enzyme-extensions. We would deprecate and drop it there if renderProp makes it into enzyme itself.

I wrote this article a while ago detailing how to use renderProp and why.

I'm curious for feedback of whether you'd want to accept it or not after some interest was shown in commercetools/enzyme-extensions#8.

Thanks to @tdeekens who helped set up and bounce ideas back&forth with for renderProp in enzyme-extensions!

Closes #1444. Related to #1856.

ljharb

Thanks!

Let's add this same API to mount, for consistency.

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

packages/enzyme/src/ShallowWrapper.js

ljharb · 2018-10-16T07:07:21Z

packages/enzyme/src/ShallowWrapper.js

+   * @returns {ShallowWrapper}
+   */
+  renderProp(propName, ...args) {
+    const Wrapper = props => props.children;


This won't work in React 13.

packages/enzyme/src/ShallowWrapper.js

ljharb · 2018-10-16T07:08:24Z

packages/enzyme/src/ShallowWrapper.js

+        throw new Error(`no prop called “${propName}“ found`);
+      }
+      if (typeof prop !== 'function') {
+        throw new Error(


this should be a TypeError

I changed it to a TypeError.

I'm curious why you'd consider this one a TypeError, but not the one above as both are problems of the component's props?

ljharb · 2018-10-16T07:09:03Z

packages/enzyme/src/ShallowWrapper.js

+      }
+
+      const element = prop(...args);
+      const wrapped = getAdapter(this[OPTIONS]).createElement(Wrapper, null, element);


we may need to add a method to all the adapters "wrap", so that the adapter itself can handle the wrapper implementation.

Do you want to do it in this PR or separately? Separately I assume, just double-checking.

I think it'd need to be in this PR (in a separate commit, ofc).

[enzyme-adapter-utils] [new] add `wrapWithSimpleWrapper`

dferber90

I tried to add wrap to all adapters but I'm not familiar enough with the internals of enzyme to get the tests to pass. I still keep bumping into TypeError: inst.render is not a function on React 14 😞 Other versions of React seem to be passing the tests now though!

Let's add this same API to mount, for consistency.

I'm holding off adding renderProp to mount until the tests are passing in shallow. I haven't used mount myself yet either. I thought mount renders the whole tree, in which case the render prop would get executed automatically? I guess I need to play with mount a bit to understand why/how it would be useful there.

packages/enzyme/src/ShallowWrapper.js

dferber90 · 2018-10-16T18:57:41Z

packages/enzyme/src/ShallowWrapper.js

+        throw new Error(`no prop called “${propName}“ found`);
+      }
+      if (typeof prop !== 'function') {
+        throw new Error(


I changed it to a TypeError.

I'm curious why you'd consider this one a TypeError, but not the one above as both are problems of the component's props?

dferber90 · 2018-10-16T21:12:12Z

I tried to add wrap to all adapters but I'm not familiar enough with the internals of enzyme to get the tests to pass. I still keep bumping into TypeError: inst.render is not a function on React 14 😞 Other versions of React seem to be passing the tests now though!

The issue was const stub = sinon.stub().returns(null); effectively rendering null from a stateless functional component, which is not supported in React 14. Changing it to const stub = sinon.stub().returns(<div />); solved the issue. This comment tipped me off.

packages/enzyme-adapter-react-13/src/ReactThirteenAdapter.js

ljharb · 2018-10-20T07:45:22Z

I've updated and rebased the branch; I'll give it another review and then we can add the method for mount as well.

docs/api/ShallowWrapper/renderProp.md

dferber90 · 2018-10-21T20:31:26Z

I'm still a bit puzzled by how renderProp would work with mount - or what it would be useful for (I haven't actually used mount myself). Depending on the system under test, the renderProp will likely get called anyways during rendering, so users can test the final output.

The only useful case for having renderProp in mount I could come up with would be to influence the arguments the actual render-prop gets called with. But there should be other means to do so by setting the parent component up properly. I'm questioning how useful having renderProp in mount would be after all.

I attempted to add mount anyways, but I failed at coming up with a good example for docs/tests and at actually making the implementation work. I'm not sure how to proceed as I feel kinda lost without understanding the end-goal of having renderProp in mount.

It would be nice if you could give me some direction or take over from here.

ljharb · 2018-10-21T21:06:51Z

It's so you can unit-test the render prop without needing to know all the permutations of props that lead to certain input combinations.

ljharb

I went ahead and added mount support, as well as some unit tests for adapters' wrap method.

This looks good to go, thanks!

dferber90 · 2018-11-06T08:31:10Z

@ljharb Thanks for driving this home!

I visited a friend this weekend so I didn't get to continue working on it. I really appreciate your help!

We'll deprecate the renderProp function in @commercetools/enzyme-extensions once this is released.

dferber90 · 2018-11-06T08:48:58Z

The current API works like this wrapper.renderProp(propName, arg1, arg2).

This will call the prop propName on wrapper with arg1 and arg2, like wrapper[propName](arg1, arg2).

This API is nice to work with but prevents us from ever adding something like an options object or any other param to renderProp itself, as all arguments after the first one get forwarded to the render prop.

It might be a better idea to use an API like wrapper.renderProp(propName, [arg1, arg2]) or wrapper.renderProp(propName)(arg1, arg2).

So far this has not been a problem for us but I wanted to give a heads-up now that it's becoming official. I'm undecided of which API is suited best.

ljharb · 2018-11-06T15:41:39Z

@dferber90 those are really good points. The explicit array is cleaner, until you want no arguments and you want to pass options. The “return a function” approach is cleaner when you have arguments, but perhaps less so when you don’t.

I think I’m slightly leaning towards the “returning a function” API. I’ll be releasing the adapters sooner than enzyme itself, so we have a bit of time to make the change, but not much :-)

dferber90

@ljharb I reviewed your latest changes and found a left-over console.log and a mistake in the docs I made. I opened #1890 which fixes them.

dferber90 · 2018-11-06T16:23:10Z

docs/api/ReactWrapper/renderProp.md

+```jsx
+const wrapper = mount(<App />)
+  .find(Mouse)
+  .renderProp('render', [10, 20]);


This is under "multiple arguments", so the code should be .renderProp('render', 10, 20)

dferber90 · 2018-11-06T16:23:40Z

docs/api/ShallowWrapper/renderProp.md

+```jsx
+const wrapper = shallow(<App />)
+  .find(Mouse)
+  .renderProp('render', [10, 20]);


Same here, this should also be .renderProp('render', 10, 20).

dferber90 · 2018-11-06T16:24:35Z

packages/enzyme-adapter-utils/src/wrapWithSimpleWrapper.jsx

+)();
+
+export default function wrap(element) {
+  console.log(React.version, Wrapper);


Is this a debugging-leftover? This should go away as far as I can tell.

whoops, yes, that's my bad.

dferber90 · 2018-11-06T17:02:28Z

@ljharb I now also opened #1891 to suggest the "return a function" approach instead.

- [new] add `wrapWithSimpleWrapper` (#1863, #1890)

- [new] `wrap`: add `wrap` to all adapters (#1863) - [deps] update `enzyme-adapter-utils`, `react-is`

dferber90 mentioned this pull request Oct 11, 2018

Why create this, without any issues or PRs to enzyme itself? commercetools/enzyme-extensions#8

Closed

dferber90 changed the title ~~[New] : add renderProp~~ [New] : add renderProp to ShallowWrapper Oct 11, 2018

dferber90 changed the title ~~[New] : add renderProp to ShallowWrapper~~ Aadd renderProp to ShallowWrapper Oct 11, 2018

dferber90 changed the title ~~Aadd renderProp to ShallowWrapper~~ Add renderProp to ShallowWrapper Oct 11, 2018

dferber90 mentioned this pull request Oct 11, 2018

Module parse failed: You may need an appropriate loader to handle this file type. commercetools/enzyme-extensions#9

Closed

This comment has been minimized.

Sign in to view

ljharb added API: shallow semver: minor New stuff. labels Oct 16, 2018

ljharb requested changes Oct 16, 2018

View reviewed changes

dferber90 added 2 commits October 16, 2018 21:32

[enzyme-adapter-react-*] [new] wrap: add wrap to all adapters

8494d0e

[enzyme-adapter-utils] [new] add `wrapWithSimpleWrapper`

[New] shallow: add renderProp

ec66aaf

dferber90 commented Oct 16, 2018

View reviewed changes

packages/enzyme-adapter-react-13/src/ReactThirteenAdapter.js Show resolved Hide resolved

ljharb reviewed Oct 20, 2018

View reviewed changes

docs/api/ShallowWrapper/renderProp.md Show resolved Hide resolved

ljharb added package: react adapter: 16 package: react adapter: 15 package: react adapter: 15.4 package: react adapter: 0.14 package: react adapter: 0.13 package: enzyme-adapter-utils package: react adapter: 16.1 package: react adapter: 16.2 package: react adapter: 16.3 labels Oct 20, 2018

[New] mount: add renderProp

d725261

ljharb added the API: mount label Nov 5, 2018

ljharb approved these changes Nov 5, 2018

View reviewed changes

ljharb merged commit 6cdd27c into enzymejs:master Nov 5, 2018

ljharb mentioned this pull request Nov 5, 2018

Complete React 16 support #1553

Open

41 tasks

dferber90 mentioned this pull request Nov 6, 2018

renderProp followup (doc corrections & console.log removal) #1890

Merged

dferber90 commented Nov 6, 2018

View reviewed changes

dferber90 mentioned this pull request Nov 6, 2018

Change renderProp API #1891

Merged

ljharb added a commit that referenced this pull request Nov 8, 2018

[enzyme-adapter-utils] v1.9.0

64573bb

- [new] add `wrapWithSimpleWrapper` (#1863, #1890)

ljharb added a commit that referenced this pull request Nov 8, 2018

[enzyme-adapter-react-{14,15.4,15}] v1.2.0

1303788

- [new] `wrap`: add `wrap` to all adapters (#1863) - [deps] update `enzyme-adapter-utils`, `react-is`

lnhrdt mentioned this pull request Jan 17, 2019

@types/enzyme - missing type definition for renderProp DefinitelyTyped/DefinitelyTyped#32234

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add renderProp to ShallowWrapper #1863

Add renderProp to ShallowWrapper #1863

dferber90 commented Oct 11, 2018 •

edited by ljharb

Loading

This comment has been minimized.

ljharb left a comment

ljharb Oct 16, 2018

ljharb Oct 16, 2018

dferber90 Oct 16, 2018

ljharb Oct 16, 2018

dferber90 Oct 16, 2018 •

edited

Loading

ljharb Oct 16, 2018

dferber90 left a comment •

edited

Loading

dferber90 Oct 16, 2018

dferber90 commented Oct 16, 2018

ljharb commented Oct 20, 2018

dferber90 commented Oct 21, 2018

ljharb commented Oct 21, 2018

ljharb left a comment

dferber90 commented Nov 6, 2018

dferber90 commented Nov 6, 2018

ljharb commented Nov 6, 2018

dferber90 left a comment

dferber90 Nov 6, 2018

dferber90 Nov 6, 2018

dferber90 Nov 6, 2018

ljharb Nov 7, 2018

dferber90 commented Nov 6, 2018

Add renderProp to ShallowWrapper #1863

Add renderProp to ShallowWrapper #1863

Conversation

dferber90 commented Oct 11, 2018 • edited by ljharb Loading

This comment has been minimized.

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dferber90 Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dferber90 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dferber90 commented Oct 16, 2018

ljharb commented Oct 20, 2018

dferber90 commented Oct 21, 2018

ljharb commented Oct 21, 2018

ljharb left a comment

Choose a reason for hiding this comment

dferber90 commented Nov 6, 2018

dferber90 commented Nov 6, 2018

ljharb commented Nov 6, 2018

dferber90 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dferber90 commented Nov 6, 2018

dferber90 commented Oct 11, 2018 •

edited by ljharb

Loading

dferber90 Oct 16, 2018 •

edited

Loading

dferber90 left a comment •

edited

Loading