fix: replace series function used to queue async callbacks by karlsander · Pull Request #11485 · react-navigation/react-navigation

karlsander · 2023-07-20T09:07:34Z

Motivation

This function is used to ensure that all state change callbacks to sync the local browser history state run in series. The functions are async because history.go is not a synchronous function.

I noticed that in chrome, the URL was updating to the previous url instead of the new one in some navigations and tracked it down to the fact that the history.go navigation takes longer than in other browsers. This function is supposed to mitigate that, but I believe it has a bug. The function will not properly return early, because the finally clause will still always run and immediately reset the isRunning state. Even after fixing this, the function didn't quite work right for queuing more than two callbacks.

I have replaced the function with a simpler implementation that does the same task.

Test plan

Describe the steps to test this change so that a reviewer can verify it. Provide screenshots or videos if the change affects UI.

The change must pass lint, typescript and tests.

github-actions · 2023-07-20T09:07:49Z

Hey @karlsander! Thanks for opening your first pull request in this repo. If you haven't already, make sure to read our contribution guidelines.

netlify · 2023-07-20T09:09:30Z

✅ Deploy Preview for react-navigation-example ready!

Name	Link
🔨 Latest commit	`ae9956d`
🔍 Latest deploy log	https://app.netlify.com/sites/react-navigation-example/deploys/64f5a1a19ab9080008e13350
😎 Deploy Preview	https://deploy-preview-11485--react-navigation-example.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

satya164

Thanks for the PR

satya164 · 2023-07-31T12:24:55Z

This change seems to break unit tests. Can you take a look?

karlsander · 2023-08-01T15:58:56Z

I'm not sure exactly, but the test is failing on main for me in the same way. I think it is addressed here? #11509

mountiny · 2023-08-22T12:23:12Z

@karlsander Would you be able to merge main to your branch? The tests are passing on main

karlsander · 2023-08-22T13:59:19Z

I can see they succeed on the branch check CI, but when I clone react-navigation/react-navigation@main and run yarn && yarn test, they still fail in the same way.

adamgrzybowski · 2023-08-24T11:34:51Z

Hey, @karlsander I've tried wrapping expect with waitFor and tests seem to work fine. Here's how it looks. https://gist.github.com/adamgrzybowski/7839818fb1a9728052bab0a1d84464e4

There's one thing I'm not 100% sure of. I had to remove is.useFakeTimers() but the tests still passed.

karlsander · 2023-08-25T09:34:12Z

The tests do technically pass, but I think only because the failure is not captured correctly, because I still get this in the logs

 node:internal/process/promises:289
            triggerUncaughtException(err, true /* fromPromise */);
            ^

JestAssertionError: expect(received).toBe(expected) // Object.is equality

Expected: "/feed"
Received: "/"
    at Object.<anonymous> (/Users/karl/code/react-navigation/packages/native/src/__tests__/NavigationContainer.test.tsx:107:28)
    at Generator.next (<anonymous>)
    at asyncGeneratorStep (/Users/karl/code/react-navigation/node_modules/@babel/runtime/helpers/asyncToGenerator.js:3:24)
    at _next (/Users/karl/code/react-navigation/node_modules/@babel/runtime/helpers/asyncToGenerator.js:22:9)
    at /Users/karl/code/react-navigation/node_modules/@babel/runtime/helpers/asyncToGenerator.js:27:7
    at new Promise (<anonymous>)
    at Object.<anonymous> (/Users/karl/code/react-navigation/node_modules/@babel/runtime/helpers/asyncToGenerator.js:19:12)
    at Promise.then.completed (/Users/karl/code/react-navigation/node_modules/jest-circus/build/utils.js:289:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (/Users/karl/code/react-navigation/node_modules/jest-circus/build/utils.js:222:10)
    at _callCircusTest (/Users/karl/code/react-navigation/node_modules/jest-circus/build/run.js:248:40)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at _runTest (/Users/karl/code/react-navigation/node_modules/jest-circus/build/run.js:184:3)
    at _runTestsForDescribeBlock (/Users/karl/code/react-navigation/node_modules/jest-circus/build/run.js:86:9)
    at run (/Users/karl/code/react-navigation/node_modules/jest-circus/build/run.js:26:3)
    at runAndTransformResultsToJestFormat (/Users/karl/code/react-navigation/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:120:21)
    at jestAdapter (/Users/karl/code/react-navigation/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19)
    at runTestInternal (/Users/karl/code/react-navigation/node_modules/jest-runner/build/runTest.js:367:16)
    at runTest (/Users/karl/code/react-navigation/node_modules/jest-runner/build/runTest.js:444:34)
    at Object.worker (/Users/karl/code/react-navigation/node_modules/jest-runner/build/testWorker.js:106:12) {
  matcherResult: {
    actual: '/',
    expected: '/feed',
    message: '\x1B[2mexpect(\x1B[22m\x1B[31mreceived\x1B[39m\x1B[2m).\x1B[22mtoBe\x1B[2m(\x1B[22m\x1B[32mexpected\x1B[39m\x1B[2m) // Object.is equality\x1B[22m\n' +
      '\n' +
      'Expected: \x1B[32m"/\x1B[7mfeed\x1B[27m"\x1B[39m\n' +
      'Received: \x1B[31m"/"\x1B[39m',
    name: 'toBe',
    pass: false
  }
}

I think waitFor also needs to be awaited before the next act, and in that case, the failure remains the same as ever. Maybe this code is actually broken, not the tests.

adamgrzybowski · 2023-08-28T12:07:26Z

@karlsander Hey I created a PR to test new tests for the series without distracting you if something went wrong 😄

I've used await and it seems to work fine. I've also checked with the debugger and the value for window.location.pathname matches the expected value.

BTW we used your series implementation as a patch in our app and it seems to solve one problem that we had so I think you did a good job with this solution

BTW_2.0 the old implementation of series is probably broken because of the try {...} finally {...} blocks. If we remove them the result is the same as with your simpler implementation (the problem in our app is fixed and the tests fail)

The finally block is executed even if you return in try and that breaks the queue. At least that's my guess

karlsander · 2023-08-29T09:43:27Z

BTW_2.0 the old implementation of series is probably broken because of the try {...} finally {...} blocks. If we remove them the result is the same as with your simpler implementation (the problem in our app is fixed and the tests fail)

I don't remember exactly, but I think in that case two async functions at the same time are queued correctly, but not 3+ (a situation that doesn't really happen in this usage anyway).

karlsander · 2023-08-29T10:22:06Z

@satya164 all looks green now, but I cannot merge because I don't have access

codecov-commenter · 2023-08-31T22:27:08Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.67%. Comparing base (fab2cc6) to head (ae9956d).
⚠️ Report is 736 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11485      +/-   ##
==========================================
+ Coverage   75.60%   75.67%   +0.06%     
==========================================
  Files         194      194              
  Lines        5785     5775      -10     
  Branches     2276     2274       -2     
==========================================
- Hits         4374     4370       -4     
+ Misses       1364     1358       -6     
  Partials       47       47

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

osdnk · 2023-09-02T20:45:02Z

@adamgrzybowski @karlsander I guess that is safe to go?

mountiny · 2023-09-03T08:24:03Z

I think so, we are using it in our App without any issues found so far 🤞 https://github.com/Expensify/App/pull/25989/files

karlsander · 2023-09-04T09:20:15Z

We are also using the patch with success, would appreciate if we can get this merged.

**Motivation** This function is used to ensure that all state change callbacks to sync the local browser history state run in series. The functions are async because `history.go` is not a synchronous function. I noticed that in chrome, the URL was updating to the previous url instead of the new one in some navigations and tracked it down to the fact that the `history.go` navigation takes longer than in other browsers. This function is supposed to mitigate that, but I believe it has a bug. The function will not properly return early, because the finally clause will still always run and immediately reset the `isRunning` state. Even after fixing this, the function didn't quite work right for queuing more than two callbacks. I have replaced the function with a simpler implementation that does the same task. **Test plan** Describe the **steps to test this change** so that a reviewer can verify it. Provide screenshots or videos if the change affects UI. The change must pass lint, typescript and tests. --------- Co-authored-by: Michał Osadnik <micosa97@gmail.com>

replace series function used to queue async callbacks

50c4eb2

github-actions Bot added the first pull request label Jul 20, 2023

karlsander changed the title ~~replace series function used to queue async callbacks~~ fix: replace series function used to queue async callbacks Jul 20, 2023

satya164 approved these changes Jul 31, 2023

View reviewed changes

adamgrzybowski mentioned this pull request Aug 22, 2023

[HOLD for payment 2023-09-11] [HOLD for payment 2023-09-08] [$1000] CTRL+K, CTRL+SHIFT+K and CTRL+K again closes everything Expensify/App#25527

Closed

6 tasks

Merge branch 'main' into fix-async-event-sequence

b8488f5

Ollyws mentioned this pull request Aug 24, 2023

Fixing the race condition on history.go due to buggy queue async callbacks Expensify/App#25866

Merged

59 tasks

update tests

82acbee

Merge branch 'main' into fix-async-event-sequence

a4cb05f

Merge branch 'main' into fix-async-event-sequence

03fe15b

Merge branch 'main' into fix-async-event-sequence

ae9956d

osdnk enabled auto-merge (squash) September 4, 2023 09:21

osdnk merged commit d8dc693 into react-navigation:main Sep 4, 2023

Uh oh!

Uh oh!

Conversation

karlsander commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 20, 2023

Uh oh!

netlify Bot commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for react-navigation-example ready!

Uh oh!

satya164 left a comment

Choose a reason for hiding this comment

Uh oh!

satya164 commented Jul 31, 2023

Uh oh!

karlsander commented Aug 1, 2023

Uh oh!

mountiny commented Aug 22, 2023

Uh oh!

karlsander commented Aug 22, 2023

Uh oh!

adamgrzybowski commented Aug 24, 2023

Uh oh!

karlsander commented Aug 25, 2023

Uh oh!

adamgrzybowski commented Aug 28, 2023

Uh oh!

karlsander commented Aug 29, 2023

Uh oh!

karlsander commented Aug 29, 2023

Uh oh!

codecov-commenter commented Aug 31, 2023 • edited by codecov Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

osdnk commented Sep 2, 2023

Uh oh!

mountiny commented Sep 3, 2023

Uh oh!

karlsander commented Sep 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

karlsander commented Jul 20, 2023 •

edited

Loading

netlify Bot commented Jul 20, 2023 •

edited

Loading

codecov-commenter commented Aug 31, 2023 •

edited by codecov Bot

Loading