mzgoddard / preact-render-spy

Render preact components with access to the produced virtual dom for testing.
https://www.npmjs.com/package/preact-render-spy
ISC License
178 stars 24 forks source link

.output() API Naming Bikeshed #66

Open gnarf opened 7 years ago

gnarf commented 7 years ago

We currently have .output() which returns a VNode, but the more I look at this, the more I think it might of been a mistake to not return a FindWrapper instance here. We need a method that can return the FindWrapper version of component output, and we've been talking about it over in #57 PR comments.

I wouldn't be entirely against going to a 2.0 and making .output() return a FindWrapper, and .nodes() returning VNodes or something, but if we can come up with a good name for outputWrapped() we might not need to break back-compat.

gnarf commented 7 years ago

So the current issue is that since we store both the "requested vnode" and the "rendered output" of the node, we have another "depth" that enzyme doesn't.

We expose the ability to find the intermediates, which I think is cool, however it leads to this problem, using the new proposed .name() method as an example:

const TestMe = () => <span>So Tested</span>

const context = shallow(<TestMe />);
// this is a little unexpected for some, one would think you'd get the "rendered" version
context.name()  === 'TestMe';
// if we try to get at it using .output() we get stuck with a VNode, not a FindWrapper
// so .name() doesn't work anymore
context.output().nodeName === 'span';

// what we want is something like:
context.resolved().name() === 'span';

We just need a good name / concept to describe this phenomenon?

developit commented 7 years ago

I was going to suggest resolved()!

So to clarify - one method returns the component-resolved tree in a FindWrapper, the other returns the component-resolved tree as VNodes?

gnarf commented 7 years ago

Correct, one allowing you to hit it with further preact-render-spy API methods for comfort, and the other in case you want to compare / snapshot the VNodes

gnarf commented 7 years ago

another example of when this would be useful:


const click = jest.fn();
const TestClick = () => <span onClick={click} />

const context = shallow(<TestClick />);
// doesn't do anything because the click is on the span, not the TestClick
context.simulate('click') 
// so we need a way to get to the <span> output!
mzgoddard commented 7 years ago

I think this will work well with something else I've been thinking about this week. We should split up the FindWrapper methods into "core" and "extended" methods. (I'll be opening a separate issue for discussion on that.) This would let us implement the ResolvedWrapper with the same "core" interface that the "extended" methods use.


@gnarf Your example would work with either the current or new API. In either API you need a wrapper containing the virtual or resolved vnode.

mzgoddard commented 7 years ago

Mentioned proposed change. https://github.com/mzgoddard/preact-render-spy/issues/67

mzgoddard commented 7 years ago

My earlier comment used the resolved name @developit mentioned. That isn't me saying I think that is the name we should use. But it is a name I think we could use. Just picked the name to help make the rest of my comment make sense.

mzgoddard commented 7 years ago

To respond a bit more to the naming question, I think the proposed names in the snippets output and resolved are good. I'm game to move forward with those. Not sure what names I'd propose otherwise have to think on it more.