Findwrapper name #57

Deseteral · 2017-10-20T15:18:53Z

Resolves #56

gnarf

Couple of small things, but overall - thanks for the PR!

gnarf · 2017-10-24T15:23:13Z

src/preact-render-spy.js

+    verifyOnlySingleNode(this);
+
+    const nodeName = this[0].nodeName;
+    return typeof nodeName === 'function'


Please don't return the result of a ternary, I'd generally prefer to see

if (typeof nodeName === 'function') { return ...; } return ...;

But that might just be my old jQuery style preferences, I think it's easier to understand if's than ternaries 90% of the time as a reader.

Also, we need to be aware of the displayName:

func.displayName should be used if it exists, like func.displayName || func.name I think.

For some reason I thought preact doesn't support displayName. I will fix that :)

gnarf · 2017-10-24T15:25:21Z

src/preact-render-spy.js

@@ -164,6 +164,12 @@ const verifyFoundNodes = wrapper => {
  }
 };

+const verifyOnlySingleNode = wrapper => {


🥇 thanks for this, we needed it

Is it weird if I ask you open a PR with this method and using it in existing methods? That'd be easy to merge in.

Sure thing :)

Take a look at #69. When we merge that PR I will merge master to this branch :)

gnarf · 2017-10-24T15:26:47Z

README.md

@@ -166,6 +166,10 @@ Returns a new `FindWrapper` with a subset of the previously selected elements gi

 Uses the same possible selectors as [`RenderContext#find(selector)`](#rendercontextfindselector).

+### `FindWrapper#name()`
+Returns the name of node.
+This can only be called on a wrapper of a single node.


Can you think of a small usage/test assertion to include here?

Deseteral · 2017-10-24T18:25:00Z

Fixed, take a look

gnarf · 2017-10-24T18:25:27Z

README.md

+}
+
+const context = shallow(<MyComponent />);
+expect(context.name()).toBe('Node');


can you verify this shouldn't be context.output().name() ? I feel like context.name() === 'MyComponent'

You're right. Now this is different behaviour than what's implemented in enzyme.
In enzyme the example from README would work.

I don't really know what to do with that. Do you have any ideas?

We aren't really trying to be 100% enzyme compatible, just "inspired by enzyme"

In many situations I could see you wanting context.output() by default, but since this is a different rendering example than say:

shallow(<div><MyComponent class="first" /><MyComponent class="second" /></div>);

where context.name() would be 'div' either way, .find('.first').name() is MyComponent, and .find('.first').output().name() would then be "Node" I think it makes sense to show the difference in the example here, just show expect(context.name()).toBe('MyComponent') AND expect(context.output().name()).toBe('Node') though .output() is returning raw VDOM instead of a FindWrapper... this puts us in an interesting place... we need an .output() that returns FindWrappers

So what you're saying is that the implementation is good (not the same as enzyme's but this is not an issue for us) and I only have to adjust the documentation?

I'm saying we probably need to figure out a solution for .outputWrapped or something to make context.rendered().name() or something workable for this test... got any thoughts? I like the idea of .output() == FindWrappered and .rendered() == VNodes - but it breaks back-compat with 1.0 already, so if we keep .output() === VNodes maybe use .resolved() === FindWrappered ?

I would be in favor of .instance() == VNode and .output() == FindWrapper but it also breaks backwards compatibility.

Or without breaking back compat make .instance() return VNode, leave .output() as it is (doing effectively the same thing as .instance()) and then make .instanceWrapper() return FindWrapper.

I'm gonna bring this conversation to #66 -- need @mzgoddard to chime in on it too I think.

I'm game for name returning this[0].nodeName or 'MyComponent'.

I think a name to see the next level in for a component node could be inspect or stepIn. I think instance would be confusing along side component.

In that case .stepIn().name() would give you 'Node' and .resolved().name() would give 'span' or whatever raw dom node name Node returns. .stepIn().stepIn().name() would also give 'span' if Node was the last component type in the chain of nested nodes.

This readme is still "incorrect" - could you update this / show an example of the difference between enzyme?

mzgoddard · 2017-10-26T16:44:33Z

src/preact-render-spy.js

@@ -164,6 +164,12 @@ const verifyFoundNodes = wrapper => {
  }
 };

+const verifyOnlySingleNode = wrapper => {
+  if (wrapper.length !== 1) {
+    throw new Error('preact-render-spy: component method can only be used on a single node');


const verifyOnlySingleNode = (wrapper, methodName) => {

throw new Error(`preact-render-spy: ${methodName} method can only be used on a single node`);

Waiting for #69 to be merged.

Is there anything else blocking this PR from being merged?

Deseteral added 3 commits October 20, 2017 17:02

FindWrapper#name implementation

4dffec9

verify only single node util

38f0f93

FindWrapper#name documentation

fe1268e

gnarf requested changes Oct 24, 2017

View reviewed changes

Deseteral added 2 commits October 24, 2017 20:03

FindWrapper#name usage example

217cd3e

FindWrapper#name displayName support

7ac95be

gnarf reviewed Oct 24, 2017

View reviewed changes

gnarf mentioned this pull request Oct 26, 2017

.output() API Naming Bikeshed #66

Open

mzgoddard mentioned this pull request Oct 26, 2017

Separate FindWrapper core and extended methods #67

Open

5 tasks

mzgoddard requested changes Oct 26, 2017

View reviewed changes

Merge branch 'master' into findwrapper-name

129172e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Findwrapper name #57

Findwrapper name #57

Deseteral commented Oct 20, 2017

gnarf left a comment

gnarf Oct 24, 2017

gnarf Oct 24, 2017

developit Oct 24, 2017

Deseteral Oct 24, 2017

gnarf Oct 24, 2017

mzgoddard Oct 26, 2017

Deseteral Oct 26, 2017

Deseteral Oct 26, 2017

Deseteral Oct 27, 2017

gnarf Oct 24, 2017

Deseteral Oct 24, 2017

Deseteral commented Oct 24, 2017

gnarf Oct 24, 2017

Deseteral Oct 24, 2017

gnarf Oct 24, 2017

Deseteral Oct 25, 2017

gnarf Oct 25, 2017

Deseteral Oct 26, 2017

gnarf Oct 26, 2017

mzgoddard Oct 26, 2017

gnarf May 16, 2018

mzgoddard Oct 26, 2017

Deseteral Oct 26, 2017

Deseteral Oct 27, 2017

Findwrapper name #57

Are you sure you want to change the base?

Findwrapper name #57

Conversation

Deseteral commented Oct 20, 2017

gnarf 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deseteral commented Oct 24, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment