Rewrite ReactBrowserEventEmitter-test #11713

madeinfree · 2017-11-30T09:58:39Z

Rewrite ReactBrowserEventEmitter-test.js to public api

- ~~~How do I setEnabled from public API ? like `ReactBrowserEventEmitter.setEnabled(false);`~~~
- ~~~is the onTouchTap ignore ?~~~

~~~Sorry I got some problem.., I didn't done yet, I would like to know how can I do on this~~~

Edit: I thought again and just change ReactBrowserEventEmitter, onTouchTap to use react-dom public path, I would like to check this change is correct or not, comment code is I'm writing now ~ please let me know, if this rewrite have any problem, I'll do my best to learn how to change it, thanks !

gaearon · 2017-11-30T18:13:03Z

packages/react-dom/src/__tests__/ReactBrowserEventEmitter-test.js

+  beforeEach(() => {
+    LISTENER.mockClear();
+
+    EventPluginHub = require('events/EventPluginHub');


Event* are internal modules. They shouldn't be here.

gaearon

Let's move any tests related to TapEventPlugin to a separate file, e.g. TapEventPlugin-test.internal.js.

gaearon · 2017-11-30T19:28:47Z

packages/react-dom/src/__tests__/ReactBrowserEventEmitter-test.js

@@ -63,6 +64,7 @@ describe('ReactBrowserEventEmitter', () => {
    EventPluginRegistry = require('events/EventPluginRegistry');
    React = require('react');
    ReactDOM = require('react-dom');
+    ReactDOMComponentTree = require('react-dom/src/client/ReactDOMComponentTree');


This is also an internal module, and tests shouldn't rely on it.

madeinfree · 2017-11-30T23:19:33Z

I rewrite the test, but remove the EventPluginRegistry cause the last test fail, do I write the test here ?

In the test, use Component instance and ref to test, I didn't find any document way to find the instance by stateless function component, like internal api ReactDOMComponentTree.getInstanceFromNode, so I use the way below, I feel weird.. is it OK ?

const trees = ReactTestUtils.findAllInRenderedTree(instance, () => true);
return EventPluginHub.getListener(inst, eventName);
return trees[3].props[eventName];

gaearon · 2017-12-01T00:05:57Z

Don't try to just "convert" the tests, some can't be written the same way. Think about what from user's perspective is being tested. Then come up with a way to test the same thing, but perhaps in a different way.

madeinfree · 2017-12-01T06:28:07Z

Thanks for tips ! I need som time to think how to rewrite it ~

madeinfree · 2017-12-01T19:56:54Z

Hi @gaearon I rewrite the test, please help me review it, thank you !

and a question the last test should work with event plugins with dependencies is EventPluginRegistry internal test ?

gaearon · 2017-12-01T20:04:42Z

packages/react-dom/src/__tests__/ReactBrowserEventEmitter-test.js

+     * @return {Component Instance} Child instance
+     */
+    getChildInstance = inst => {
+      return ReactTestUtils.findRenderedComponentWithType(inst, Child);


Can we use refs and avoid the need for this method?

oh ! I missing refs can use with Component Class before I read the document again, I remove the get* method and only use refs

gaearon · 2017-12-01T20:33:10Z

packages/react-dom/src/__tests__/ReactBrowserEventEmitter-test.js

    ReactBrowserEventEmitter.setEnabled(false);
-    const child = getChildInstance(inst);
-    ReactTestUtils.SimulateNative.click(ReactDOM.findDOMNode(child));
+    ReactTestUtils.SimulateNative.click(ReactDOM.findDOMNode(CHILD));


Suggestion: let's remove any use of Simulate and SimulateNative in this file.

Instead, make sure that we add container to document.body in beforeEach, remove it from document.body in afterEach, and then use browser native dispatch:

node.dispatchEvent( new MouseEvent('click', {bubbles: true, cancelable: true}), );

madeinfree · 2017-12-01T21:14:51Z

OK, I remove all Simulate and use new MouseEvent to instead, but I found the toThrowError in test can't catch the error log

Edit:
Hi @gaearon please help me review it, thanks !

gaearon · 2017-12-07T18:49:16Z

packages/react-dom/src/__tests__/ReactBrowserEventEmitter-test.js

+
+    React = require('react');
+    ReactDOM = require('react-dom');
+    ReactBrowserEventEmitter = require('react-dom/src/events/ReactBrowserEventEmitter');


This is still a private API. The point of this rewrite is to remove reliance on private APIs.

gaearon

Please remove remaining dependencies on ReactBrowserEventEmitter. It doesn't matter how it's imported: it's still an internal module.

gaearon · 2017-12-07T22:36:11Z

packages/react-dom/src/__tests__/ReactBrowserEventEmitter-test.internal.js

@@ -54,7 +40,6 @@ describe('ReactBrowserEventEmitter', () => {
    jest.resetModules();
    LISTENER.mockClear();

-    // TODO: can we express this test with only public API?
    EventPluginHub = require('events/EventPluginHub');


I'm not sure if I was being clear enough.

Anything except react or react-dom themselves is considered private API. Nothing that starts with events/ or has /src/ in the path is public.

gaearon

Seems like there were more tests earlier? If you meant to delete some of them intentionally please explain why.

I’d love to get closer to accepting this. But if you’re struggling with something, you need to say. Just deleting tests makes my job harder because now I need to verify which ones you deleted and whether there was a good reason (or if you just couldn’t convert them). It is better if you post comments explaining your reasoning, and if you have problems, point them out and we can brainstorm them together.

madeinfree · 2017-12-08T08:48:06Z

Hi, @gaearon thank for your reply ~

I saw your reply yesterday and I'm not ready this PR yet, I would have comment some question after I get off work but I just thinking how could I ask 😟

some confused about test for me from ReactBrowserEventEmitter internal method,
examples:

test for should not invoke handlers if ReactBrowserEventEmitter is disabled

//... test code
ReactBrowserEventEmitter.setEnabled(false);
//... test code

the ReactBrowserEventEmitter.setEnabled(false); seems like the internal behavior,
if I want to test from public API, I don't know how to test it, or I should use browser native API to instead it ?

test for should listen to events only once

spyOnDevAndProd(EventTarget.prototype, 'addEventListener');
ReactBrowserEventEmitter.listenTo('onClick', document);
ReactBrowserEventEmitter.listenTo('onClick', document);
expect(EventTarget.prototype.addEventListener.calls.count()).toBe(1);

Edit:
this seems to test the internal can only bind once, I try use addEventListener('click') to test it but got test failed

test for should work with event plugins with dependencies

// ...test code
var module = EventPluginRegistry.registrationNameModules['onChange'];
// ...test code

I confused in this line EventPluginRegistry.registrationNameModules['onChange'];,
if I want to use registrationNameModules, I don't know how to add it from public API

I reference some rewrite PR and found some public API doc but I still confused now, I'll keep to thinking how could I do, if have any suggestion please let me know 🙏🙏🙏,

Super thanks to you, Dan.

gaearon · 2017-12-08T08:54:28Z

the ReactBrowserEventEmitter.setEnabled(false); seems like the internal behavior

Try to find when it gets set :-) Then simulate the same situation. If I recall correctly it gets set when we’re already inside an event handler. So you probably want to fire an event, and then fire another event inside an event handler.

madeinfree · 2017-12-08T09:10:55Z

OK I'll keep going !

timjacobi · 2017-12-12T11:00:00Z

The event system is also disabled while a component is being unmounted.

madeinfree · 2017-12-17T14:28:02Z

Sorry, I'm busy on my work this week, I'll keep to rewrite next week ~~

philipp-spiess · 2018-08-11T10:22:50Z

@madeinfree Thank you so much for looking into that! I've seen your comment in the umbrella issue: #11299 (comment). It's absolutely no problem if you have no more time to work on it. I'm sure a future contributor will find this PR very helpful and can take over from there.

Do you think we can close your PR or do you plan to give it another try?

madeinfree · 2018-08-11T10:35:27Z

Thanks @philipp-spiess
I'll try to continue it, keep going to find helpful solution.

philipp-spiess · 2018-08-11T10:48:23Z

@madeinfree Awesome! So from a quick glimpse you can try to do the following to simulate the behavior where the react browser emitter is disabled:

Dispatch an event from within an event handler.
Dispatch an event while a component is being unmounted (componentWillUnmount maybe).

There are also some merge conflicts so you might want to rebase first.

* remove legacy code.

* comment and preapre to separate TapEventPlugin-test.internal.js

* merge rebase code

* a little change

* remove all old version test * rewrite all test

* remove get* method * only use refs

* appendChild container on document.body * remove container after each test * remove all Simulate * use new MouseEvent to dispatch native browser Can not catch the error from new MouseEvent in test 'toThrowError'.

* remove ReactBrowserEventEmitter internal lib * add ReactBrowserEventEmitter is disabled test

* update ReactBrowserEventEmitter is disabled test * use unmount to test it

* plugins test * dependencies test * complete all test

* compare order and new

* move jest.fn to beforeEach

* remove unnecessary comment

* just use node.click() * rewrite ReactBrowserEventEmitter disable test

* remove resetModules

* remove updateInstance comment

* PARENT -> parentRef * CHILD -> childRef

* remove createInitialInstance comment

* createInitialInstance always return parent so we can remove parentRef

* LISTENER.mock.calls.length -> toHaveBeenCalledTimes

* use push instead concat

sizebot · 2018-09-18T14:17:43Z

Details of bundled changes.

Comparing: 42d1231...979262b

schedule

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
schedule.development.js	n/a	n/a	0 B	19.17 KB	0 B	5.74 KB	UMD_DEV
schedule.production.min.js	n/a	n/a	0 B	3.16 KB	0 B	1.53 KB	UMD_PROD

Generated by 🚫 dangerJS

* fix eslint

madeinfree · 2018-09-18T14:41:03Z

Hi @philipp-spiess I finish the check, please review again when you have time, thanks ! :-)

philipp-spiess

I started with the review up until (and including) "should bubble simply". It would be great if you work on getting those tests ready until I have more time to review the rest.

Ideally we can get rid of the abstraction of using createInitialInstance() as well as updateInstance() and just set up the smallest possible component tree in each test. E.g. a test like "should invoke a simple handler registered on a node" does not need more than one <div onClick={listener}>. Everything else makes it a lot harder to understand what's going on.

Thanks for your patience here - I really appreciate your work on this!

philipp-spiess · 2018-09-20T21:32:47Z

packages/react-dom/src/__tests__/ReactBrowserEventEmitter-test.internal.js

-
-    function Child(props) {
-      return <div ref={c => (CHILD = c)} {...props} />;
+    class Child extends React.PureComponent {


Have you found out why this PureComponent is needed? It was also set on the wrapper of Child previously - Maybe to avoid the Child from receiving updates?

philipp-spiess · 2018-09-20T21:33:46Z

packages/react-dom/src/__tests__/ReactBrowserEventEmitter-test.internal.js


 describe('ReactBrowserEventEmitter', () => {
  beforeEach(() => {
    jest.resetModules();
-    LISTENER.mockClear();
+    LISTENER = jest.fn();


I think we don't need to have LISTENER as an variable in the closure and can just call jest.fn() whenever we want a listener :-)

philipp-spiess · 2018-09-20T21:34:54Z

packages/react-dom/src/__tests__/ReactBrowserEventEmitter-test.internal.js

-            <ChildWrapper {...CHILD_PROPS} />
-          </div>
-        </div>,
+    createInitialInstance = () => {


Can we remove this abstraction (as well as updateInstance()) and use ReactDOM.render() directly in each test? This way we can make sure to use the least amount of setup for each test and make it a lot easier to follow.

philipp-spiess · 2018-09-20T21:37:20Z

packages/react-dom/src/__tests__/ReactBrowserEventEmitter-test.internal.js

+        onClick: LISTENER,
+      },
+    });
+    expect(childRef.props.onClick).toBe(LISTENER);


This test is not asserting the same thing as the previous one. The previous test used to call the internal API ReactDOMComponentTree.getInstanceFromNode(node); to find out if React internally added an event listener. The new code is asserting that the onClick prop is set which is what you do in the line above. I think this would be green even if we remove the event system.

I think this test can safely be removed though. We have a lot of coverage for simple onClick cases.

philipp-spiess · 2018-09-20T21:37:38Z

packages/react-dom/src/__tests__/ReactBrowserEventEmitter-test.internal.js

+        onClick: LISTENER,
+      },
+    });
+    expect(childRef.props.onClick).toBe(LISTENER);


This test is a duplicate to the one above, check my comments there.

philipp-spiess · 2018-09-20T21:39:39Z

packages/react-dom/src/__tests__/ReactBrowserEventEmitter-test.internal.js

+      parentProps: {},
+      childProps: {},
+    });
+    expect(childRef.props.onClick).toBe(undefined);


This test is again only asserting the props that you set. I think you can use ReactDOM.render() with an onClick listener and then another ReactDOM.render() without the listener, then fire a native event, and assert that the handler is not called. This way we know that React has cleaned up the listeners internally.

philipp-spiess · 2018-09-20T21:41:10Z

packages/react-dom/src/__tests__/ReactBrowserEventEmitter-test.internal.js

+      }
+      // The ReactBrowserEventEmitter is disabled when WillUnmount lifecycle work
+      componentWillUnmount() {
+        willUnmountCalls();


This can be removed.

stale · 2020-01-10T08:52:29Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

stale · 2020-01-17T08:59:05Z

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!

facebook-github-bot added the CLA Signed label Nov 30, 2017

madeinfree force-pushed the remove-internal-dependencies branch from e807142 to 69dced2 Compare November 30, 2017 17:25

madeinfree changed the title ~~[Question] Rewrite ReactBrowserEventEmitter-test~~ Rewrite ReactBrowserEventEmitter-test Nov 30, 2017

madeinfree closed this Nov 30, 2017

madeinfree reopened this Nov 30, 2017

gaearon reviewed Nov 30, 2017

View reviewed changes

gaearon requested changes Nov 30, 2017

View reviewed changes

madeinfree force-pushed the remove-internal-dependencies branch from f30ba20 to d5bb17d Compare November 30, 2017 18:41

gaearon reviewed Nov 30, 2017

View reviewed changes

gaearon reviewed Dec 1, 2017

View reviewed changes

gaearon reviewed Dec 7, 2017

View reviewed changes

gaearon requested changes Dec 7, 2017

View reviewed changes

gaearon reviewed Dec 7, 2017

View reviewed changes

gaearon reviewed Dec 8, 2017

View reviewed changes

madeinfree mentioned this pull request Jan 4, 2018

Express more tests via public API #11299

Closed

26 tasks

madeinfree force-pushed the remove-internal-dependencies branch from bf0bd7c to c212ff9 Compare August 11, 2018 13:18

madeinfree added 21 commits September 18, 2018 21:21

Remove: remove legacy code

88ffc35

* remove legacy code.

Migrate: prepare to move TapEventPlugin test

e3f7bfa

* comment and preapre to separate TapEventPlugin-test.internal.js

Rebase: merge rebase code

be9def3

* merge rebase code

Change: little change

5dfdb31

* a little change

Test: remove and rewrite test

2bdbbde

* remove all old version test * rewrite all test

Test: use refs

70d1090

* remove get* method * only use refs

Test: remove Simulate and use MouseEvent

a4f327a

* appendChild container on document.body * remove container after each test * remove all Simulate * use new MouseEvent to dispatch native browser Can not catch the error from new MouseEvent in test 'toThrowError'.

Test: update test

df978ba

* remove ReactBrowserEventEmitter internal lib * add ReactBrowserEventEmitter is disabled test

Test: update test

c79eea5

* update ReactBrowserEventEmitter is disabled test * use unmount to test it

Test: update test (complete all test)

c6f7b88

* plugins test * dependencies test * complete all test

test: compare older and new

402f955

* compare order and new

test: move jest.fn to beforeEach

a27a522

* move jest.fn to beforeEach

test: remove unnecessary comment

a13c0d9

* remove unnecessary comment

test: refactor test

4eb6f6d

* just use node.click() * rewrite ReactBrowserEventEmitter disable test

test: remove resetModules

da46ad6

* remove resetModules

test: remove comment

7e17d8d

* remove updateInstance comment

test: rename PARENT CHILD

081e40c

* PARENT -> parentRef * CHILD -> childRef

test: remove comment

2839952

* remove createInitialInstance comment

test: remove parentRef

00d1105

* createInitialInstance always return parent so we can remove parentRef

test: use toHaveBeenCalledTimes

37b0bbf

* LISTENER.mock.calls.length -> toHaveBeenCalledTimes

test: use push instead concat

78ce4ef

* use push instead concat

madeinfree force-pushed the remove-internal-dependencies branch from eb585cd to 78ce4ef Compare September 18, 2018 14:05

test: fix eslint

979262b

* fix eslint

philipp-spiess suggested changes Sep 20, 2018

View reviewed changes

stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020

stale bot closed this Jan 17, 2020

Rewrite ReactBrowserEventEmitter-test #11713

Rewrite ReactBrowserEventEmitter-test #11713

Conversation

madeinfree commented Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madeinfree commented Nov 30, 2017 • edited Loading

gaearon commented Dec 1, 2017

madeinfree commented Dec 1, 2017

madeinfree commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madeinfree commented Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

madeinfree commented Dec 8, 2017 • edited Loading

gaearon commented Dec 8, 2017

madeinfree commented Dec 8, 2017

timjacobi commented Dec 12, 2017

madeinfree commented Dec 17, 2017 • edited Loading

philipp-spiess commented Aug 11, 2018

madeinfree commented Aug 11, 2018 • edited Loading

philipp-spiess commented Aug 11, 2018

sizebot commented Sep 18, 2018 • edited Loading

schedule

madeinfree commented Sep 18, 2018

philipp-spiess 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

stale bot commented Jan 10, 2020

stale bot commented Jan 17, 2020

madeinfree commented Nov 30, 2017 •

edited

Loading

madeinfree commented Nov 30, 2017 •

edited

Loading

madeinfree commented Dec 1, 2017 •

edited

Loading

madeinfree commented Dec 8, 2017 •

edited

Loading

madeinfree commented Dec 17, 2017 •

edited

Loading

madeinfree commented Aug 11, 2018 •

edited

Loading

sizebot commented Sep 18, 2018 •

edited

Loading