shallow wrapper .simulate() SyntheticEvent and event propagation #368

nfcampos · 2016-05-06T21:59:38Z

What do you guys think about this? I just added an empty React SyntheticEvent as the first argument that .simulate calls event handlers with.
This provides the regular event methods (stopPropagation and friends) by default but still allows the user to replace them eg. with spies, exactly the same way they do now.

todo

make propagation go through the capture phase as well
make propagation tests tree be deeper
- assert that event handlers for other events aren't called neither on target node nor on parents
test that it successfully skips over intermediate nodes without event handler
test propagation when clicking on a 'composite' node
assert presence of stopPropagation and friends on event object passed to event handlers
guard with this.single
- test that it throws with more than one node
add propagation tests for SFCs

nfcampos · 2016-05-06T22:04:19Z

This is not a complete solution to all the shallow wrapper's event shortcomings (still no propagation) but to me it looks like a change that should be compatible with whatever ends up being done for propagation, since all it does is call event handlers with an object with an event-like interface

nfcampos · 2016-05-07T00:38:57Z

I've added a POC for event propagation :) let me know what you think
edit: which doesn't currently work if you called .simulate() on a wrapper with more than one element I believe, but that should be fixable.

nfcampos · 2016-05-07T09:11:54Z

src/ShallowWrapper.js

-    }
+  simulate(event, mock, ...args) {
+    const eventProp = propFromEvent(event);
+    const e = Object.assign(new SyntheticEvent(undefined, undefined, { type: event }), mock);


if the mock replaces the stopPropagation method event propagation stops working as intended so this Object.assign is probably not a great idea (also would have to be the object.assign npm module because of old versions of node)

aweary · 2016-05-07T15:36:47Z

src/ShallowWrapper.js

@@ -26,6 +26,7 @@ import {
  createShallowRenderer,
  renderToStaticMarkup,
 } from './react-compat';
+import SyntheticEvent from 'react/lib/SyntheticEvent';


This should likely be re-exported from ./react-compat, not directly imported from react/lib.

yeah i realised that afterwards, ill change it if this goes forward

lelandrichardson · 2016-05-08T01:04:00Z

@nfcampos thanks for this contribution! I think that this is on the right track. I have a couple of things though:

As Aweary mentioned, let's pull this inside of react-compat
If user provides explicit null as first argument to simulate, no event object ends up getting passed into the event handler.

There are a couple of questions this raises:

Is this a breaking change? this will likely make a lot of code using simulate follow new code paths that could cause breakage.
There is now no way to simulate a custom event (ie, onCustomThingHappens) where the data being passed into that is not supposed to be an event object, but rather some data object that is domain specific. How do we preserve the ability to simulate events and pass in arbitrary data structures instead of synthetic events?

Unfortunately my first .simulate() API wasn't forward-thinking enough to forecast these backwards compatibility issues.

@ljharb do you have any thoughts?

My thinking is that this will have to be a breaking change, and that perhaps we need to provide an additional way for users to specify not using the synthetic event.

nfcampos · 2016-05-08T09:38:02Z

@lelandrichardson I've pulled the import of SyntheticEvent into react-compat.

I think there's no way around this being a breaking change, especially because of the event propagation this introduces, it will simply do stuff in your tests that it didn't do before.

A couple questions:

does it make sense for the .simulate method to support custom events? We at least need to make it clear to the user that even custom events will follow the propagation logic
In the spirit of making the shallow wrapper have an api as similar to the full rendering wrapper as possible, I don't think it makes sense for the .simulate method to:
1. support custom events (the full rendering wrapper doesn't https://github.com/airbnb/enzyme/blob/master/src/ReactWrapper.js#L384)
2. support arbitrary event data (the full rendering wrapper doesn't https://github.com/facebook/react/blob/master/src/test/ReactTestUtils.js#L522 the passed in arg is Object.assign'ed into the SyntheticEvent)
3. support more arguments beyond the first (the full rendering wrapper doesn't https://github.com/facebook/react/blob/master/src/test/ReactTestUtils.js#L493 , this is in fact a mistake in our wrapper around the TestUtils simulate, we send more arguments which are ignored, https://github.com/airbnb/enzyme/blob/master/src/ReactWrapper.js#L387)
like the full rendering .simulate the shallow simulate should be guarded with this.single to prevent you simulating an event on more than one node at a time (you can always do that explicitly with forEach, but we shouldn't magically 'dispatch' several events on a single call to .simulate, do you agree?
if we do want to support custom events, custom args I feel it should be a separate method (but IMO we should do that only if we added the same method to the full rendering wrapper) (ie. something like

callHandler(event,  ...args) {
  // do exactly what shallow::simulate did before this PR
}

nfcampos · 2016-05-08T10:50:27Z

another thing, I think there should be a command in package.json to run tests without jsdom, to make sure shallow doesn't depend on window being available. Ideally each test file that needs jsdom would import it but since mocha runs all test files in the same process it would end up being available in all the tests...

lelandrichardson · 2016-05-08T16:30:03Z

src/react-compat.js

@@ -134,6 +134,8 @@ if (REACT013) {
  };
 }

+import SyntheticEvent from 'react/lib/SyntheticEvent';


nit: please move this to the top of the file. import statements get executed before anything else when transpiled by babel anyway (and in the spec), so putting this here can be misleading.

I agree. I've moved it to the top

lelandrichardson · 2016-05-08T16:56:25Z

@nfcampos all good points. I think you are correct, the only way out of this is to create another method that handles non-standard event simulation.

To reiterate your points, I think we should move forward with the following guides:

.simulate(event, mock) should have semantically equivalent behavior for both shallow and mount (to the extent possible, consider shallow is only a shallow tree).
An additional method (how about invoke(name, ...args)?) will be provided that will allow for custom events, and not handle propogation

Further questions i have:

what about the capture phase? do we want to simulate onClickCapture?
If i have a <Button /> component with an onClick handler, are we still allowing for button.simulate('click')?

Note: I also agree that this should be wrapped inside of this.single 👍

nfcampos · 2016-05-08T17:11:35Z

If i have a component with an onClick handler, are we still allowing for button.simulate('click')?

@lelandrichardson is this b4014e0 what you mean? If yes, yeah that still works.

nfcampos · 2016-05-08T17:14:00Z

what about the capture phase? do we want to simulate onClickCapture?

If you think it's worth it, I can add that, it's doable, do you think it's worth it?

lelandrichardson · 2016-05-08T17:15:13Z

If you think it's worth it, I can add that, it's doable, do you think it's worth it?

I think that if we decide we need it later that it will need to be another breaking change, so I think it's worth it for us to get it right this time.

nfcampos · 2016-05-08T17:19:42Z

Yeah I tend to agree. Then there is one other thing we should think about now: events that have exotic behaviour (I'm not sure what to call it) eg. calling click on a <button type='submit' /> will call submit on the parent form. Should we also support this? Are there other behaviours like this? I'm only aware of this one

lelandrichardson · 2016-05-08T17:22:38Z

^ I Think @ljharb will have some thoughts on this.

The main thing we are going to face is that it is extremely hard to emulate this behavior reliably on a shallow wrapper because by design we are looking at an incomplete picture of the DOM.

Thinking about this further, we are realistically going to have the same problem with click/capture event phases because we have no way of knowing if a parent component ends up preventing the capture phase or not.

nfcampos · 2016-05-08T17:31:14Z

Hmm, re: capturing I think it's doable, think of eg. this tree

<div onClickCapture={e => e.stopPropagation()}>
  <a onClickCapture={innerOnClick}>foo</a>
</div>

If click on a is simulated we first traverse the whole tree from the target, a, up until the root, in this case only a then div, then call the capture handlers on each (if they exist) in the opposite order, starting with the div. (and then the regular handlers starting from the target). Since the div stops propagation then we bail out of further calls and never call innerOnClick.
Am I missing something?

Obviously we only know whether a parent node that you actually rendered stops propagation or not, when you end up using the component you're testing in your app it might be inside a component that stops propagation. But that's the whole point of unit testing components individually, no?

lelandrichardson · 2016-05-08T18:00:22Z

Yes the capturing will work so long as you're not worries about effects from outside your component. This is reasonable, but is definitely not the whole picture. This is an instance where there is a sort of implicit public contract of the component that we are not able to test. In reality we could provide an API to capture events at the root node of the wrapper, and then the tester could mock out the "response" of the parent component, which could be preventing the capture phase.

These are definitely corner cases, but perhaps something we should consider nonetheless.

nfcampos · 2016-05-08T18:21:04Z

So this is in a way similar to context in which a parent component changes the behaviour of its children implicitly (ie. through something other than props), right?
Any other instance of that sort of situation?
I'm thinking the clearest way to test stuff like this would be to able to specify a root component to render the component you're testing inside of. Such a component could provide context, or register capture event handlers or do whatever else might change the behaviour of the component you're trying to test. This seems the cleanest way to allow testing this kind of things without bloating the API.

lelandrichardson · 2016-05-08T18:26:57Z

Yeah, although props and context are somewhat similar (props are just less explicit) in that they are top => down. Event bubbling is in the opposite direction and seems trickier to formulate an API that makes sense around it.

nfcampos · 2016-05-08T18:50:40Z

Bubbling is not that hard to test, the only way an outside component can mess with a component's behaviour through event bubbling is if that component accepts children and the children stop event propagation. But you're already free to test components with whatever children you may like, it's just another prop.
Capturing is the difficult one, because it's implicit, it all happens outside the component you're testing. I'm not sure what the best thing for that is. Allowing a custom root component to render your component in allows you to test whatever you want but makes tests more verbose, you have to create a component that does whatever it is you're testing.

lelandrichardson · 2016-05-08T21:48:21Z

The API I was thinking would be something like:

const click = spy();
const capture = spy();
const Foo = () => (<div><a onClick={click} onClickCapture={capture} /></div>);
const wrapper = shallow(<Foo />);
// register a listener for events bubbling up, (or down in the case of the capture phase)
wrapper.on('click', e => { e.stopPropogation(); });
wrapper.find('a').simulate('click');
expect(click).to.have.callCount(1);
expect(capture).to.have.callCount(0);

.on() might be a bit too general of a name for this API, but maybe something like onEventBubble('click', handler) or something.

nfcampos · 2016-05-08T22:17:05Z

@lelandrichardson actually in that example I think neither click nor capture would get called, stopping propagation on the capture phase skips the bubbling phase as a whole, since it happens before. Correct if I'm wrong, but this .on method would be useful only for registering capture handlers, right? seeing as it registers a root handler for the event, a root bubble phase handler would be useless, it would fire after all other event handlers...

aweary · 2016-11-12T19:03:54Z

@nfcampos @ljharb any blockers here?

ljharb · 2016-11-12T19:23:38Z

I'll take another look shortly; either way let's hold off merging in this breaking change (and the others queued up) until we've merged in all the non-majors and cut a release.

ljharb · 2016-11-12T19:27:35Z

docs/api/ShallowWrapper/invoke.md

+    const { count } = this.state;
+    return (
+      <div>
+        <div className={`clicks-${count}`}>


not actually a blocker ofc, but i'd prefer to use a data attribute here, so that the example follows best practices :-)

You're right, I've changed it

blainekasten · 2016-12-19T13:20:59Z

I'll take another look shortly; either way let's hold off merging in this breaking change (and the others queued up) until we've merged in all the non-majors and cut a release.

Non-majors keep coming in. Maybe we make a project of what to batch in the next major release so we can actually act on this.

- invokes event handlers on selected node with custom arguments and no propagation - added tests for invoke - added invoke to docs

nfcampos · 2017-05-17T18:49:57Z

@ljharb I was trying to rebase this and it seems with react 15.5 SyntheticEvent is no longer importable from react, so I'm not sure where to go from here.

ljharb · 2017-05-17T21:12:05Z

@nfcampos they likely moved it to another package? if not, then we should file an issue on react asking for it to be exported.

nfcampos · 2017-08-31T18:24:53Z

@ljharb thinking about this some more, I don't think we want to start depending on SyntheticEvent, seeing as that is React-specific and we're going in the opposite direction in v3. What do you suggest we do with this, drop it or somehow not use SyntheticEvent?

ljharb · 2017-08-31T22:31:35Z

I've come to believe that .simulate is an inherently problematic API, and shouldn't be used at all - so I'm not sure it's worth enhancing it.

nfcampos · 2017-09-01T06:55:01Z

Why problematic?

ljharb · 2017-09-01T07:04:18Z

It conveys a false promise - it doesn't actually simulate anything. There's not necessarily an event, there's not necessarily bubbling, there's not any chain of events (like if you simulate a change, you don't get all the interim key events).

Whereas if you use .prop('foo')(), you explicitly invoke a function, and you know what arguments you're passing it.

nfcampos · 2017-09-26T19:30:17Z

@ljharb I tend to agree, closing

nfcampos changed the title ~~[RFC] shallow wrapper .simulate calls event handlers with SyntheticEvent~~ [RFC] shallow wrapper .simulate calls event handlers with SyntheticEvent (and propagation...) May 7, 2016

nfcampos reviewed May 7, 2016
View reviewed changes

nfcampos changed the title ~~[RFC] shallow wrapper .simulate calls event handlers with SyntheticEvent (and propagation...)~~ [RFC] shallow wrapper .simulate() SyntheticEvent and event propagation May 7, 2016

nfcampos mentioned this pull request May 7, 2016

.simulate(event) doesn't properly propagate events in ShallowWrapper #3

Open

aweary reviewed May 7, 2016
View reviewed changes

nfcampos force-pushed the events branch from 10ddaff to 6920d0d Compare May 8, 2016 09:01

lelandrichardson reviewed May 8, 2016
View reviewed changes

nfcampos force-pushed the events branch from 681a086 to 4b632c7 Compare May 8, 2016 17:01

nfcampos force-pushed the events branch from b4014e0 to 8855eb9 Compare May 8, 2016 18:21

lelandrichardson mentioned this pull request May 8, 2016

Add default events object to shallow wrapper simulate #329

Closed

nfcampos force-pushed the events branch from a570567 to a50914a Compare November 12, 2016 12:10

ljharb approved these changes Nov 12, 2016

View reviewed changes

nfcampos force-pushed the events branch from a50914a to 3865d8b Compare November 13, 2016 09:59

Nuno Campos and others added 14 commits March 19, 2017 11:04

added SyntheticEvent

2d175f7

added event propagation

8444af7

added test for event triggered on composite element

0d7e4dc

added this.single

a48aeca

added capture phase to propagation

3cb4f5a

added more tests

43a7df4

added shallow-mount parity test

5bb7240

added test for batched updates + event propagation

98e20bc

added tests for SFCs

4475a19

updated docs

5bcce8d

added invoke method to ShallowWrapper

e8a3623

- invokes event handlers on selected node with custom arguments and no propagation - added tests for invoke - added invoke to docs

add method name to this.single invocation for invoke() and simulate()

82ea16d

linting

4018993

address comment

0105d5b

nfcampos force-pushed the events branch from 72e919e to 0105d5b Compare March 19, 2017 11:05

nfcampos mentioned this pull request May 17, 2017

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

Merged

nfcampos closed this Sep 26, 2017

shallow wrapper .simulate() SyntheticEvent and event propagation #368

shallow wrapper .simulate() SyntheticEvent and event propagation #368

Conversation

nfcampos commented May 6, 2016 • edited

todo

nfcampos commented May 6, 2016

nfcampos commented May 7, 2016 • edited

nfcampos May 7, 2016 • edited

Choose a reason for hiding this comment

aweary May 7, 2016 • edited

Choose a reason for hiding this comment

nfcampos May 7, 2016

Choose a reason for hiding this comment

lelandrichardson commented May 8, 2016

nfcampos commented May 8, 2016 • edited

nfcampos commented May 8, 2016

lelandrichardson May 8, 2016

Choose a reason for hiding this comment

nfcampos May 8, 2016

Choose a reason for hiding this comment

lelandrichardson commented May 8, 2016

nfcampos commented May 8, 2016 • edited

nfcampos commented May 8, 2016

lelandrichardson commented May 8, 2016

nfcampos commented May 8, 2016

lelandrichardson commented May 8, 2016

nfcampos commented May 8, 2016 • edited

lelandrichardson commented May 8, 2016

nfcampos commented May 8, 2016 • edited

lelandrichardson commented May 8, 2016

nfcampos commented May 8, 2016

lelandrichardson commented May 8, 2016

nfcampos commented May 8, 2016

aweary commented Nov 12, 2016

ljharb commented Nov 12, 2016

ljharb Nov 12, 2016

Choose a reason for hiding this comment

nfcampos Nov 13, 2016

Choose a reason for hiding this comment

blainekasten commented Dec 19, 2016

nfcampos commented May 17, 2017

ljharb commented May 17, 2017

nfcampos commented Aug 31, 2017

ljharb commented Aug 31, 2017

nfcampos commented Sep 1, 2017

ljharb commented Sep 1, 2017

nfcampos commented Sep 26, 2017

nfcampos commented May 6, 2016 •

edited

nfcampos commented May 7, 2016 •

edited

nfcampos May 7, 2016 •

edited

aweary May 7, 2016 •

edited

nfcampos commented May 8, 2016 •

edited

nfcampos commented May 8, 2016 •

edited

nfcampos commented May 8, 2016 •

edited

nfcampos commented May 8, 2016 •

edited