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

Add renderProp to ShallowWrapper #1863

Merged
merged 3 commits into from Nov 5, 2018

Conversation

@dferber90
Copy link
Contributor

dferber90 commented Oct 11, 2018

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.

@dferber90 dferber90 force-pushed the dferber90:master branch from a9fd4e9 to 7469819 Oct 11, 2018

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

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

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

@dferber90

This comment was marked as resolved.

Copy link
Contributor

dferber90 commented Oct 11, 2018

The tests are fine for React >= 15. However, I keep getting errors in React 0.13 and 0.14, and I'm not sure how to resolve them. Maybe somebody has seen them before and knows what to do?

React 0.14

  1) shallow .renderProp() returns a ShallowWrapper wrapper around the node returned from the render prop:
     TypeError: inst.render is not a function
      at ShallowComponentWrapper._renderValidatedComponentWithoutOwnerOrContext (node_modules/react/lib/ReactCompositeComponent.js:587:34)
      at ShallowComponentWrapper.mountComponent (node_modules/react/lib/ReactCompositeComponent.js:220:30)
      at ShallowComponentWrapper.wrapper [as mountComponent] (node_modules/react/lib/ReactPerf.js:66:21)
      at ReactShallowRenderer._render (node_modules/react/lib/ReactTestUtils.js:366:14)
      at _batchedRender (node_modules/react/lib/ReactTestUtils.js:348:12)
      at ReactDefaultBatchingStrategyTransaction.perform (node_modules/react/lib/Transaction.js:136:20)
      at Object.batchedUpdates (node_modules/react/lib/ReactDefaultBatchingStrategy.js:62:19)
      at Object.batchedUpdates (node_modules/react/lib/ReactUpdates.js:94:20)
      at ReactShallowRenderer.render (node_modules/react/lib/ReactTestUtils.js:343:16)
      at packages/enzyme-adapter-react-14/build/ReactFourteenAdapter.js:1:1
      at withSetStateAllowed (packages/enzyme-adapter-utils/build/Utils.js:1:1)
      at Object.render (packages/enzyme-adapter-react-14/build/ReactFourteenAdapter.js:1:1)
      at new ShallowWrapper (packages/enzyme/build/ShallowWrapper.js:1:1)
      at ShallowWrapper.wrap (packages/enzyme/build/ShallowWrapper.js:1:1)
      at ShallowWrapper.<anonymous> (packages/enzyme/build/ShallowWrapper.js:1:1)
      at ShallowWrapper.single (packages/enzyme/build/ShallowWrapper.js:1:1)
      at ShallowWrapper.shallow (packages/enzyme/build/ShallowWrapper.js:1:1)
      at ShallowWrapper.<anonymous> (packages/enzyme/build/ShallowWrapper.js:1:1)
      at ShallowWrapper.single (packages/enzyme/build/ShallowWrapper.js:1:1)
      at ShallowWrapper.renderProp (packages/enzyme/build/ShallowWrapper.js:1:1)
      at Context.<anonymous> (packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx:4759:26)
      at process.topLevelDomainCallback (domain.js:121:23)

React 0.13

1) shallow .renderProp() returns a ShallowWrapper wrapper around the node returned from the render prop:
     TypeError: Cannot add property context, object is not extensible
      at ShallowComponentWrapper.mountComponent (node_modules/react/lib/ReactCompositeComponent.js:153:18)
      at ShallowComponentWrapper.wrapper [as mountComponent] (node_modules/react/lib/ReactPerf.js:70:21)
      at ReactShallowRenderer._render (node_modules/react/lib/ReactTestUtils.js:391:14)
      at ReactShallowRenderer.render (node_modules/react/lib/ReactTestUtils.js:375:8)
      at ReactShallowRenderer.contextCompatibleRender [as render] (packages/enzyme-adapter-react-13/build/ReactThirteenAdapter.js:1:1)
      at Object.render (packages/enzyme-adapter-react-13/build/ReactThirteenAdapter.js:1:1)
      at new ShallowWrapper (packages/enzyme/build/ShallowWrapper.js:1:1)
      at ShallowWrapper.wrap (packages/enzyme/build/ShallowWrapper.js:1:1)
      at ShallowWrapper.<anonymous> (packages/enzyme/build/ShallowWrapper.js:1:1)
      at ShallowWrapper.single (packages/enzyme/build/ShallowWrapper.js:1:1)
      at ShallowWrapper.shallow (packages/enzyme/build/ShallowWrapper.js:1:1)
      at ShallowWrapper.<anonymous> (packages/enzyme/build/ShallowWrapper.js:1:1)
      at ShallowWrapper.single (packages/enzyme/build/ShallowWrapper.js:1:1)
      at ShallowWrapper.renderProp (packages/enzyme/build/ShallowWrapper.js:1:1)
      at Context.<anonymous> (packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx:4752:33)
      at process.topLevelDomainCallback (domain.js:121:23)

@dferber90 dferber90 force-pushed the dferber90:master branch from 7469819 to f663a8e Oct 12, 2018

@ljharb
Copy link
Member

ljharb left a comment

Thanks!

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

Show resolved Hide resolved packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx Outdated
Show resolved Hide resolved packages/enzyme/src/ShallowWrapper.js Outdated
* @returns {ShallowWrapper}
*/
renderProp(propName, ...args) {
const Wrapper = props => props.children;

This comment has been minimized.

@ljharb

ljharb Oct 16, 2018

Member

This won't work in React 13.

Show resolved Hide resolved packages/enzyme/src/ShallowWrapper.js Outdated
throw new Error(`no prop called “${propName}“ found`);
}
if (typeof prop !== 'function') {
throw new Error(

This comment has been minimized.

@ljharb

ljharb Oct 16, 2018

Member

this should be a TypeError

This comment has been minimized.

@dferber90

dferber90 Oct 16, 2018

Contributor

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?

}

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

This comment has been minimized.

@ljharb

ljharb Oct 16, 2018

Member

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

This comment has been minimized.

@dferber90

dferber90 Oct 16, 2018

Contributor

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

This comment has been minimized.

@ljharb

ljharb Oct 16, 2018

Member

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

@dferber90 dferber90 force-pushed the dferber90:master branch 2 times, most recently from 728f2db to 615d76d Oct 16, 2018

[enzyme-adapter-react-*] [new] `wrap`: add `wrap` to all adapters
[enzyme-adapter-utils] [new] add `wrapWithSimpleWrapper`
@dferber90
Copy link
Contributor

dferber90 left a comment

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.

Show resolved Hide resolved packages/enzyme/src/ShallowWrapper.js Outdated
throw new Error(`no prop called “${propName}“ found`);
}
if (typeof prop !== 'function') {
throw new Error(

This comment has been minimized.

@dferber90

dferber90 Oct 16, 2018

Contributor

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

This comment has been minimized.

Copy link
Contributor

dferber90 commented Oct 16, 2018

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.

@ljharb ljharb force-pushed the dferber90:master branch from b4cde86 to 2e81336 Oct 20, 2018

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Oct 20, 2018

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

@dferber90 dferber90 force-pushed the dferber90:master branch from 2e81336 to 32f67c1 Oct 21, 2018

@dferber90

This comment has been minimized.

Copy link
Contributor

dferber90 commented Oct 21, 2018

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

This comment has been minimized.

Copy link
Member

ljharb commented Oct 21, 2018

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 ljharb force-pushed the dferber90:master branch from 32f67c1 to 43fda4b Nov 5, 2018

@ljharb ljharb added the Mount label Nov 5, 2018

@ljharb ljharb force-pushed the dferber90:master branch 4 times, most recently from 4512f08 to d725261 Nov 5, 2018

@ljharb

ljharb approved these changes Nov 5, 2018

Copy link
Member

ljharb left a comment

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

This looks good to go, thanks!

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ljharb ljharb referenced this pull request Nov 5, 2018

Open

Complete React 16 support #1553

18 of 38 tasks complete
@dferber90

This comment has been minimized.

Copy link
Contributor

dferber90 commented Nov 6, 2018

@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

This comment has been minimized.

Copy link
Contributor

dferber90 commented Nov 6, 2018

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

This comment has been minimized.

Copy link
Member

ljharb commented Nov 6, 2018

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

dferber90 left a comment

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

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

This comment has been minimized.

@dferber90

dferber90 Nov 6, 2018

Contributor

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

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

This comment has been minimized.

@dferber90

dferber90 Nov 6, 2018

Contributor

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

)();

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

This comment has been minimized.

@dferber90

dferber90 Nov 6, 2018

Contributor

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

This comment has been minimized.

@ljharb

ljharb Nov 7, 2018

Member

whoops, yes, that's my bad.

@dferber90 dferber90 referenced this pull request Nov 6, 2018

Merged

Change renderProp API #1891

@dferber90

This comment has been minimized.

Copy link
Contributor

dferber90 commented Nov 6, 2018

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

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

[enzyme-adapter-utils] v1.9.0
 - [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
 - [new] `wrap`: add `wrap` to all adapters (#1863)
 - [deps] update `enzyme-adapter-utils`, `react-is`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment