Add ShallowWrapper#invoke() to invoke event handlers and return the handlers value #945

milesj · 2017-05-16T23:02:32Z

This updates ShallowWrapper#simulate() to return the value of the executed handler if available. This allows us to easily test changes that occur in the next tick, like promise chains.

// handler
onClick() {
  return Promise.resolve()
    .then(() => {
      this.setState({ foo: true });
    });
}

// test
wrapper.simulate('click').then(() => {
  expect(wrapper.state('foo')).toBe(true);
});

ljharb · 2017-05-16T23:38:23Z

src/ShallowWrapper.js

      });
    }
-    return this;
+    return null;


this should return undefined rather than null.

ljharb · 2017-05-16T23:39:18Z

src/ShallowWrapper.js

        performBatchedUpdates(this, () => {
-          handler(...args);
+          response = handler(...args);


I wonder if it would be better to return the equivalent of Promise.try(() => handler(...args)), to cleanly encapsulate any exceptions - the only downside being that you couldn't get at the result synchronously. Are there use cases for a synchronous non-Promise return value?

ljharb · 2017-05-16T23:39:50Z

src/Utils.js

@@ -218,13 +218,14 @@ export function withSetStateAllowed(fn) {
    cleanup = true;
    global.document = {};
  }
-  fn();
+  const response = fn();


return try { fn(); } finally { if (cleanup) { // This works around a bug in node/jest in that developers aren't able to // delete things from global when running in a node vm. global.document = undefined; delete global.document; } };

(this is arguably what it should have been doing all along, in case fn throws)

nfcampos · 2017-05-17T17:11:15Z

Linking to two previous PRs doing similar things #867 #898

As per the discussion on the other two PRs, events don't have a single result, because they fire zero, one or more event listeners. (so if you start exposing the return value of a handler, which of the several handlers do you choose?) The fact that our current implementation of ShallowWrapper#simulate does not properly ~~propagate~~ bubble events, thus firing at most a single event handler, is a shortcoming that we probably want to address at some point (see #368 for an implementation of just that).

As such, this gets a 👎 from me, what do the other maintainers think?

cc @ljharb

nfcampos · 2017-05-17T17:20:10Z

In other words, if we get this in we're accepting that the simulate method on shallow will be semantically different from the simulate method on mount going forward, and we should close #368

milesj · 2017-05-17T17:31:03Z

@nfcampos In the context of a React component, there is only a single event with a single return value, so that shouldn't be an issue.

But yes, this would deviate mount and shallow. Perhaps we can add a new method and keep existing functionality, like simulateAndReturn?

nfcampos · 2017-05-17T17:36:27Z

@milesj events in browsers bubble through the tree of rendered elements, ie. in

<div onClick={doSomethingElse}>
  <div onClick={doSomething} />
</div>

if you click on the inner div both onClick handlers fire.
Our current implementation of shallow happens to not respect that (mount does) so currently if you do .simulate('click') on that inner div using the shallow renderer only doSomething will be invoked.

milesj · 2017-05-17T17:39:54Z

Correct, and that's a single event. That's what I'm getting at with shallow.

nfcampos · 2017-05-17T17:40:56Z

yes, it's a single event, invoking two event handlers, so how do you choose which one to return the return value for?

milesj · 2017-05-17T17:55:11Z

Shallow doesn't invoke 2 event handlers -- you literally just said this above. It executes a prop of the same name, for the element that is currently found. That is 1 event with 1 return value.

We're talking in the context of shallow, not mount.

nfcampos · 2017-05-17T18:00:39Z

yes, and as I said above, that's something we've been thinking about changing for quite some time (see open PR at https://github.com/airbnb/enzyme/pull/368/files#diff-cc80d1de64f4765743cbe5179e51ff36, or in fact just look at @lelandrichardson's comment right above one of the lines you changed https://github.com/airbnb/enzyme/pull/945/files#diff-00a0c52d2a0eac5ec0025c1d32767a95R624).

So, re-iterating, if we do bring your change in, we'll have to abandon our previous effort at introducing event propagation for shallow.simulate because the two concepts are not compatible.

I'm not strictly against either option, but I do believe we should have that trade-off in mind while considering either option.

milesj · 2017-05-17T18:05:37Z

Which is why I suggested an additional method that doesn't use propagation.

I'd wager that triggering a prop function on a React component has much higher usage than triggering an actual event.

nfcampos · 2017-05-17T18:07:59Z

like https://github.com/airbnb/enzyme/pull/368/files#diff-13d31897d7e0bc58111b00a5b11bee40 ?

milesj · 2017-05-17T18:25:19Z

Yup. Would just need to return the value, or be wrapped in a promise, or else we'd still have the same next tick problems.

nfcampos · 2017-05-17T18:29:45Z

do you want to cherry-pick the relevant commits out of that PR and change the return value?

milesj · 2017-05-17T20:57:36Z

I can look into it since that PR is currently blocked.

milesj · 2017-06-01T21:18:20Z

@nfcampos @ljharb @kesne

Updated the PR to pull invoke from #368

Also noticed there were no docs for isEmptyRender, so added them here.

ljharb

Should we add invoke to ReactWrapper as well?

ljharb · 2017-06-01T21:22:16Z

docs/api/ReactWrapper/isEmptyRender.md

@@ -0,0 +1,18 @@
+# `.isEmptyRender() => Boolean`
+
+Returns whether or not the current component returns a falsey value: `false`, `null`, `undefined`.


s/falsey/falsy/g

(altho actually i'll just cherry-pick this into master now and fix it inline; you can rebase after that and omit this commit)

milesj · 2017-06-01T22:44:53Z

@ljharb It looks like simulate functionality from React test utils does not return the handlers value, so we're unable to do that at this time.

ljharb · 2017-06-01T22:48:09Z

(you may want to retry the rebase, and this time excise the isEmptyRender commit entirely)

milesj · 2017-06-01T22:52:07Z

@ljharb Rebased the last few commits and squashed them. Looks good now.

ljharb

Looks great!

It would be worth looking into what it would take (including rewriting simulate on ReactWrapper) to add invoke to mount.

ljharb · 2017-06-01T22:56:11Z

test/ShallowWrapper-spec.jsx

+              <a
+                className={`clicks-${this.state.count}`}
+                onClick={this.incrementCount}
+              >foo</a>


nbd, especially in a test, but i'd expect the linter to error out here and require:

<a className={`clicks-${this.state.count}`} onClick={this.incrementCount} > foo </a>

Perhaps the eslint config is out of date?

maybe, i also might be wrong and the linter doesn't catch this yet :-) no big deal tho, it'll get fixed eventually

milesj · 2017-06-02T17:20:34Z

@ljharb This is where the simulate happens from test-utils: https://github.com/facebook/react/blob/master/src/renderers/dom/test/ReactTestUtils.js#L442

Events are queued and executed as a batch. Doesn't look easy to pull the handler value out of it. https://github.com/facebook/react/blob/master/src/renderers/shared/shared/event/EventPluginHub.js#L224

ljharb · 2017-06-03T06:13:32Z

I wonder if we did something like, invoke react's simulate method, but not on the prop value - instead, on something like (...args) => { const value = originalPropValue(...args); return value; }?

For example, this could be done by temporarily mutating the prop value on the component, and then after the simulation was done, restoring it? The batched updates may make it harder tho.

milesj · 2017-06-06T20:00:09Z

@ljharb When using React's simulate, we pass a DOM node, not the props. I'm not sure that will work.

nfcampos

LGTM other than two minor comments

nfcampos · 2017-06-11T21:47:25Z

docs/api/ShallowWrapper/invoke.md

+
+#### Returns
+
+`Any`: Returns the value from the event handler..


two dots at the end

nfcampos · 2017-06-11T21:48:09Z

docs/api/shallow.md

@@ -172,6 +172,9 @@ Returns the key of the current node.
 #### [`.simulate(event[, data]) => ShallowWrapper`](ShallowWrapper/simulate.md)
 Simulates an event on the current node.

+#### [`.invoke(event[, ...args]) => Any`](ShallowWrapper/invoke.md)
+Invokes an event handler on the current node and returns the handlers value.


“and returns the handler’s return value.”

nfcampos · 2017-06-11T21:52:49Z

is anything stopping us from implementing this in mount in the exact same way, ie. select the prop out of the component and invoke it?

koba04 · 2017-12-30T16:10:19Z

Currently, .simulate() causes many problems so I think enzyme should have an alternative API to replace simulate().
I think this invoke() feels to be close it.
What status is this PR?

ljharb · 2017-12-30T17:52:34Z

docs/api/ShallowWrapper/invoke.md

@@ -0,0 +1,39 @@
+# `.invoke(event[, ...args]) => Any`
+
+Invokes an event handler (a prop that matches the event name).


i kind of wonder if maybe this should just be a prop name rather than an event name, ie, onClick, not click? Then it can be used to invoke any function-valued prop.

Yes I think I agree

+1 for onClick rather than click.

milesj · 2017-12-31T09:24:51Z

This kind of got lost in the version 3 release. I'll dig into it again at some point.

ljharb · 2018-08-29T06:51:20Z

@milesj would you be open to rebasing this with invoke on both shallow and mount wrappers, with this signature/behavior?

invoke(propName, ...args) {
  // throw if `this.prop(propName)` is not a function
  // call it with `args` list
  // returns the result
}

That way, we can conveniently get access to the return value, with a nicer syntax than .prop('onClick')().

Thoughts?

milesj · 2018-08-29T07:15:14Z

Yeah most definitely. Can jump on this again.

[New] shallow: Add invoke(eventName, ...args)

258823f

milesj changed the title ~~Updated ShallowWrapper#simulate() to return the handlers value~~ Update ShallowWrapper#simulate() to return the handlers value May 16, 2017

ljharb added semver: major Breaking changes API: shallow labels May 16, 2017

ljharb requested changes May 16, 2017

View reviewed changes

milesj changed the title ~~Update ShallowWrapper#simulate() to return the handlers value~~ Add ShallowWrapper#invoke() to invoke event handlers and return the handlers value Jun 1, 2017

ljharb reviewed Jun 1, 2017

View reviewed changes

ljharb approved these changes Jun 1, 2017

View reviewed changes

ljharb added semver: minor New stuff. and removed semver: major Breaking changes labels Jun 1, 2017

ljharb requested review from nfcampos, lelandrichardson, blainekasten and aweary June 7, 2017 00:00

nfcampos approved these changes Jun 11, 2017

View reviewed changes

ljharb reviewed Dec 30, 2017

View reviewed changes

koba04 mentioned this pull request Oct 8, 2018

Invoke() api #1856

Merged

milesj closed this Mar 7, 2019

ljharb reopened this Mar 7, 2019

ljharb merged commit 258823f into enzymejs:master Apr 6, 2019

milesj deleted the simulate-return branch April 6, 2019 23:25

		@@ -0,0 +1,18 @@
		# `.isEmptyRender() => Boolean`

		Returns whether or not the current component returns a falsey value: `false`, `null`, `undefined`.


		#### Returns

		`Any`: Returns the value from the event handler..

		@@ -0,0 +1,39 @@
		# `.invoke(event[, ...args]) => Any`

		Invokes an event handler (a prop that matches the event name).

Add ShallowWrapper#invoke() to invoke event handlers and return the handlers value #945

Add ShallowWrapper#invoke() to invoke event handlers and return the handlers value #945

Conversation

milesj commented May 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfcampos commented May 17, 2017 • edited

nfcampos commented May 17, 2017

milesj commented May 17, 2017

nfcampos commented May 17, 2017

milesj commented May 17, 2017

nfcampos commented May 17, 2017

milesj commented May 17, 2017

nfcampos commented May 17, 2017

milesj commented May 17, 2017

nfcampos commented May 17, 2017

milesj commented May 17, 2017

nfcampos commented May 17, 2017

milesj commented May 17, 2017

milesj commented Jun 1, 2017 • edited

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

milesj commented Jun 1, 2017

ljharb commented Jun 1, 2017

milesj commented Jun 1, 2017

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

milesj commented Jun 2, 2017

ljharb commented Jun 3, 2017

milesj commented Jun 6, 2017

nfcampos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfcampos commented Jun 11, 2017 • edited

koba04 commented Dec 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milesj commented Dec 31, 2017

ljharb commented Aug 29, 2018

milesj commented Aug 29, 2018

milesj commented May 16, 2017 •

edited

nfcampos commented May 17, 2017 •

edited

milesj commented Jun 1, 2017 •

edited

nfcampos commented Jun 11, 2017 •

edited