Add test for duplicate events with nested dispatch #8566

Andarist · 2016-12-13T16:45:10Z

Still have tests to write, but they should be pretty straightforward after PR aproval.

Also had no idea where I should put once utility function, so its place in the PR for now is just a temporary.

This PR is just a proposal of a fix, the thing I've come with to tackle this problem, I'm open to change the code appropriately if somebody comes up with the better idea how to solve this.

cc @aweary

aweary · 2016-12-13T17:31:47Z

Thanks for the PR @Andarist, and for the write up in #8559. I'm wondering if we can safely just get rid of this whole dev-only abstraction. We use it elsewhere (like for componentWillUnmount) so it seems kind of strange for it to be dispatching an event for that.

gaearon · 2016-12-13T17:35:04Z

It dispatches a fake event so that errors originating in user code are on the top of the stack and "break on uncaught exceptions" works. I think that's the main reason.

Andarist · 2016-12-13T21:02:32Z

Hm, its in fact kinda weird that componentWillUnmount can use this __DEV__ version ReactErrorUtils.invokeGuardedCallback.

It seems that it should always (regardless the env) use this one -

react/src/renderers/shared/utils/ReactErrorUtils.js

Line 24 in 7ef856a

function invokeGuardedCallback<A>(

.

Maybe that should be fixed too?

aweary · 2016-12-14T15:45:43Z

We can update that call in unmountComponent to use invokeGuardedCallbackWithCatch which is the try / catch version in all environments.

As for using once here, it feels like we're addressing the symptom instead of the issue. Can we deregister the event handler in the bound function, before we call func? That way the fake event handler won't be called again in the first place.

Andarist · 2016-12-15T00:31:41Z

So I've pushed out the new version. It might seem that it contains some unnecessary changes, but:

it seems that jest.resetModuleRegistry is a deprecated or internal API (not in the docs) and it only calls documented - jest.resetModules
I've added jest.resetModules() to the beforeEach, because of the jest caching I've stumbled upon an issue with ReactErrorUtils being evaluated just once (and second force evaluation in other production-only test) per this file. So when I've put my new test after the one which forced ReactErrorUtils to be evaluated as in production mode, it has stayed the same (no re-evaluation within beforeEach) - therefore I couldnt test properly and wasted some time on figuring this out. I could change the test order, but thought this change might save somebody's time in the future. WDYT?

Also - the test's comment might not be the best, but cannot express it better atm

aweary · 2016-12-15T00:44:26Z

src/renderers/shared/utils/__tests__/ReactErrorUtils-test.js

+      ReactErrorUtils.invokeGuardedCallback('foo', callback, 'arg', true);
+      expect(callback).toHaveBeenCalledTimes(1);
+      expect(callback2).toHaveBeenCalledTimes(1);
+    });


Does this test fail if you revert the changes in ReactErrorUtils?

Yes. But test file would need to stay as in my PR so the ordering issue is solved by resetModules

Andarist · 2016-12-15T09:30:33Z

CicrcleCI didnt pass on 1 container out of 4, but aint sure why, as locally I do not have any problems and 3 containers passed the tests just fine.

Did you start using Circle recently? I dont recall it being used before.

Andarist · 2017-01-10T10:14:36Z

@aweary
i've seen that you have closed and reopened the issue - probably to force CircleCI to run the build

However it didnt pass on some of the containers (similarly like before). After that I've rebased my branch which forced another build - and the same thing again.

Do you have any idea why this might happen? My changes were really minor and I dont see why this is happening.

aweary · 2017-01-10T17:17:55Z

@Andarist don't worry, it's failing for unrelated reasons (see 378ef5e)

gaearon · 2017-01-10T18:02:37Z

(You should be able to rebase to fix this?)

Andarist · 2017-01-10T23:57:49Z

I've done that, but 1 container is still failing.

I think it's connected with ongoing fiber development and the fact I've added a test case. From what we can see overall in https://circleci.com/gh/facebook/react/760#tests/containers/2 ./scripts/circleci/test_fiber.sh is not exactly passing - just like on my branch here - https://circleci.com/gh/facebook/react/772#tests/containers/2 , but the outcome is different, master is OK, but my branch is not.

Just a wild idea (cause im not aware how the things are developed atm) - should i run scripts/fiber/record-tests now and add it to the commit? Does it make a difference?

gaearon · 2017-01-11T15:54:30Z

Just a wild idea (cause im not aware how the things are developed atm) - should i run scripts/fiber/record-tests now and add it to the commit? Does it make a difference?

Yes, exactly! Sorry it's not documented.

Andarist · 2017-01-11T16:05:15Z

Rebased + ammended with scripts/fiber/tests-passing.txt. Working like a charm and PR is green ;)

gaearon · 2017-01-11T16:33:21Z

src/renderers/shared/utils/__tests__/ReactErrorUtils-test.js

@@ -71,5 +72,15 @@ describe('ReactErrorUtils', () => {
      __DEV__ = true;
      global.process = oldProcess;
    });
+
+    it('shouldn\'t call the callback again while executing synchronously induced other call in development', () => {


Is there any way to write a test for observable behavior instead? That is, write a test that relies on ReactDOM and React, but not on ReactErrorUtils itself. Otherwise, it's hard to tell what exactly this is fixing.

Sure thing, should the new one be located in the same file or should I move it somewhere else? gonna prepare better test later 2day

ReactDOM-test might be an okay place to put it.

Andarist · 2017-01-22T23:55:59Z

Ok, so I've changed the test (removed old one too), so it should clearly indicate what's it testing against.

I had few issues with understanding how should I handle it and if any of you could clarify things a little bit it would be great.

First of all it is totally unclear for me how to cause an event, there are numerous ways to do so and had no idea which to chose, finally just have chosen the one which gave me my expected results, but I had to test a few. Anyway - not much of a problem, but overally such things could be described somewhere (or maybe ive just didnt find this?)

Also when during my experiments I've put:

beforeEach(() => {
  jest.resetModules();
  ReactDOM = require('ReactDOM');
});

it has stopped to work and I have no idea why, wasted some time on this one as in executeDispatchesInOrder dispatchListeners were falsy and no handler was called. Removing the beforeEach fixed this.

But the biggest obstacle Ive met is that I think that code during testing is going through some other path than in the latest React. There are some fiber-called things in the call stack which are absent in production code. I think this might be responsible for the different behaviour that I have wanted to debug - but ultimately failed. It might (or not) be relevant for your guys though.

The problem was that the real life situation was acting like this before the fix - handler1 > handler1 > handler2. But during tests it was handler1 > handler1 > handler1 > handler1 > ..., so effectively it created an endless loop.

My fix is working for both scenarios, but I was curious about this difference so I wanted to investigate. I THINK its caused by some logic connected to enqueueEvents - with each iteration of the dispatch loop the first handler somehow go into being accumulated there again and again and again, which was not the case with real life scenario. Hope it means something more for you than to me :)

If somebody is interested I have a call stack for this endless loop which I can post here later.

aweary

@Andarist sorry for letting this slide, there were some pretty big changes to ReactErrorUtils made in #8961. Can you verify the issue still occurs with those changes, and if so rebase your changes with those?

Andarist · 2017-03-04T10:15:56Z

@aweary
#8961 fixed this, also confirmed that just before #8961 the issue still occured

That being said v15 branch wont ever have this fix from #8961 as its fiber-related heavily. Dont know how you guys approach this kind of situations and if you diverge things for 2 different major releases.

Also the test could be incorporated into the v16 to ensure regression wont happen here in the future.

Let me know what I should do about this PR, i.e I can split this into 2 commits - 1st for v15 fix, 2nd for test, so only the latter can be cherry-picked for master, while both could be merged to v15 line.

aweary · 2017-03-21T16:49:24Z

@Andarist if #8961 fixed this then we should be good to go, we have a release manager that will deal with this and make sure changes to utilities used by 15 are included in the next release 👍

Andarist · 2017-03-22T09:55:57Z

@aweary How v15 can get this fix from #8961 ? From what I see this PR is Fiber-related, doesnt it imply this is for v16+ only?

Also what about the test? Both versions could benefit from it, guarding against regression in this matter.

aweary · 2017-03-22T14:05:27Z

ReactErrorUtils.js is a shared module so we should be able to integrate those changes into the stack renderer as well. Looking at #8961 the changes made to ReactErrorUtils don't appear incompatible with 15. But actually I think this next 15 release has to be done manually, maybe @gaearon can clarify how we'd approach this

Also what about the test? Both versions could benefit from it, guarding against regression in this matter.

Yeah definitely, I'll re-open if you can just remove the changes to ReactErrorUtils and just include the test

Andarist · 2017-03-29T17:03:36Z

@aweary
So I've rebased and ammended my change. Besides the test guarding against future regressions Ive also decided to apply also a change from original PR.

I've moved unsubscribe logic to the start of the event's callback here. I think it wont harm anything as tests are passing and it serves as additional guard - the only reason that the other refactor has fixed this issue is the fact that depth was added to the event type. Without that it would still be broken, so future changes might break it.

With this line moved to the callback, it stays double guarded by different event types + by clean up logic moved. Also it will ensure that cleaning up will actually happen in case of an error and we wont end up with stale listener.

Possibly even with such stale listener we might end up with other bugs, when some other code would dispatch new event with the same name and depth. I need to check this though and possibly write tests for such case.

Andarist · 2017-08-09T08:44:41Z

@aweary ping ;)

this is a silly PR really, but would like to have it stop hanging as open in my PR tab, I understand though if you are too busy right now

…d ref) of the same type within a handler calling an original handler too (fixes facebook#8559)

aweary · 2017-08-09T14:29:51Z

@Andarist I think we can just merge this since it's just adding additional test coverage now. Thanks for this, sorry about the huge delay 😄

Andarist · 2017-08-09T17:36:49Z

No problem, sorry that i forgot to adjust commit message while rebasing

facebook-github-bot added GH Review: review-needed CLA Signed labels Dec 13, 2016

aweary self-assigned this Dec 13, 2016

facebook-github-bot added the CLA Signed label Dec 13, 2016

Andarist force-pushed the fix/8559 branch from 9905a33 to f494e17 Compare December 14, 2016 09:13

Andarist force-pushed the fix/8559 branch from f494e17 to c2b68f8 Compare December 15, 2016 00:25

aweary reviewed Dec 15, 2016

View reviewed changes

aweary closed this Jan 9, 2017

aweary reopened this Jan 9, 2017

Andarist force-pushed the fix/8559 branch from c2b68f8 to f304daf Compare January 10, 2017 01:14

Andarist force-pushed the fix/8559 branch from f304daf to ee437ed Compare January 10, 2017 23:29

Andarist force-pushed the fix/8559 branch from ee437ed to 06007a5 Compare January 11, 2017 15:59

gaearon reviewed Jan 11, 2017

View reviewed changes

Andarist force-pushed the fix/8559 branch from 06007a5 to 0a9f1ec Compare January 22, 2017 23:39

sebmarkbage removed the GH Review: review-needed label Mar 1, 2017

aweary suggested changes Mar 2, 2017

View reviewed changes

aweary closed this Mar 21, 2017

aweary reopened this Mar 22, 2017

Andarist force-pushed the fix/8559 branch from 0a9f1ec to 7af2299 Compare March 29, 2017 16:17

Andarist force-pushed the fix/8559 branch from 7af2299 to 534c802 Compare July 12, 2017 07:33

Andarist force-pushed the fix/8559 branch from 534c802 to 3933c36 Compare August 9, 2017 08:53

Fixed DEV mode issue with synchronously caused event (i.e. by accesse…

384d082

…d ref) of the same type within a handler calling an original handler too (fixes facebook#8559)

Andarist force-pushed the fix/8559 branch from 3933c36 to 384d082 Compare August 9, 2017 09:16

aweary approved these changes Aug 9, 2017

View reviewed changes

aweary changed the title ~~Fixed DEV mode issue with synchronously caused event (i.e. by accesse…~~ Add test for duplicate events with nested dispatch Aug 9, 2017

aweary merged commit 9166cf9 into facebook:master Aug 9, 2017

Andarist deleted the fix/8559 branch August 9, 2017 17:33

Add test for duplicate events with nested dispatch #8566

Add test for duplicate events with nested dispatch #8566

Uh oh!

Conversation

Andarist commented Dec 13, 2016 • edited by aweary Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aweary commented Dec 13, 2016

Uh oh!

gaearon commented Dec 13, 2016

Uh oh!

Andarist commented Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aweary commented Dec 14, 2016

Uh oh!

Andarist commented Dec 15, 2016

Uh oh!

aweary Dec 15, 2016

Choose a reason for hiding this comment

Uh oh!

Andarist Dec 27, 2016

Choose a reason for hiding this comment

Uh oh!

Andarist commented Dec 15, 2016

Uh oh!

Andarist commented Jan 10, 2017

Uh oh!

aweary commented Jan 10, 2017

Uh oh!

gaearon commented Jan 10, 2017

Uh oh!

Andarist commented Jan 10, 2017

Uh oh!

gaearon commented Jan 11, 2017

Uh oh!

Andarist commented Jan 11, 2017

Uh oh!

gaearon Jan 11, 2017

Choose a reason for hiding this comment

Uh oh!

Andarist Jan 11, 2017

Choose a reason for hiding this comment

Uh oh!

gaearon Jan 11, 2017

Choose a reason for hiding this comment

Uh oh!

Andarist commented Jan 22, 2017

Uh oh!

aweary left a comment

Choose a reason for hiding this comment

Uh oh!

Andarist commented Mar 4, 2017

Uh oh!

aweary commented Mar 21, 2017

Uh oh!

Andarist commented Mar 22, 2017

Uh oh!

aweary commented Mar 22, 2017

Uh oh!

Andarist commented Mar 29, 2017

Uh oh!

Andarist commented Aug 9, 2017

Uh oh!

aweary commented Aug 9, 2017

Uh oh!

Andarist commented Aug 9, 2017

Uh oh!

Uh oh!

Andarist commented Dec 13, 2016 •

edited by aweary

Loading

Andarist commented Dec 13, 2016 •

edited

Loading