Skip to content
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

.output() API Naming Bikeshed #66

Open
gnarf opened this issue Oct 26, 2017 · 8 comments
Open

.output() API Naming Bikeshed #66

gnarf opened this issue Oct 26, 2017 · 8 comments

Comments

@gnarf
Copy link
Collaborator

gnarf commented Oct 26, 2017

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
Copy link
Collaborator Author

gnarf commented Oct 26, 2017

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?

@gnarf gnarf changed the title API Naming Bikeshed .output() API Naming Bikeshed Oct 26, 2017
@developit
Copy link

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
Copy link
Collaborator Author

gnarf commented Oct 26, 2017

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
Copy link
Collaborator Author

gnarf commented Oct 26, 2017

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
Copy link
Owner

mzgoddard commented Oct 26, 2017

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 mzgoddard reopened this Oct 26, 2017
@mzgoddard
Copy link
Owner

Mentioned proposed change. #67

@mzgoddard
Copy link
Owner

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants