Skip to content

Commit

Permalink
fix: replace series function used to queue async callbacks (#11485)
Browse files Browse the repository at this point in the history
**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>
  • Loading branch information
karlsander and osdnk committed Sep 4, 2023
1 parent fab2cc6 commit d8dc693
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 48 deletions.
31 changes: 11 additions & 20 deletions packages/native/src/__tests__/NavigationContainer.test.tsx
Expand Up @@ -6,7 +6,7 @@ import {
TabRouter,
useNavigationBuilder,
} from '@react-navigation/core';
import { act, render } from '@testing-library/react-native';
import { act, render, waitFor } from '@testing-library/react-native';
import * as React from 'react';

import { window } from '../__mocks__/window';
Expand All @@ -18,9 +18,7 @@ Object.assign(global, window);
// eslint-disable-next-line import/extensions
jest.mock('../useLinking', () => require('../useLinking.tsx'));

it('integrates with the history API', () => {
jest.useFakeTimers();

it('integrates with the history API', async () => {
const createStackNavigator = createNavigatorFactory((props: any) => {
const { state, descriptors, NavigationContent } = useNavigationBuilder(
StackRouter,
Expand Down Expand Up @@ -104,51 +102,44 @@ it('integrates with the history API', () => {

act(() => navigation.current?.navigate('Profile', { user: 'jane' }));

expect(window.location.pathname).toBe('/jane');
await waitFor(() => expect(window.location.pathname).toBe('/jane'));

act(() => navigation.current?.navigate('Updates'));

expect(window.location.pathname).toBe('/updates');
await waitFor(() => expect(window.location.pathname).toBe('/updates'));

act(() => navigation.current?.goBack());

jest.runAllTimers();

expect(window.location.pathname).toBe('/jane');
await waitFor(() => expect(window.location.pathname).toBe('/jane'));

act(() => {
window.history.back();
jest.runAllTimers();
});

expect(window.location.pathname).toBe('/feed');
await waitFor(() => expect(window.location.pathname).toBe('/feed'));

act(() => {
window.history.forward();
jest.runAllTimers();
});

expect(window.location.pathname).toBe('/jane');
await waitFor(() => expect(window.location.pathname).toBe('/jane'));

act(() => navigation.current?.navigate('Settings'));

expect(window.location.pathname).toBe('/edit');
await waitFor(() => expect(window.location.pathname).toBe('/edit'));

act(() => {
window.history.go(-2);
jest.runAllTimers();
});

expect(window.location.pathname).toBe('/feed');
await waitFor(() => expect(window.location.pathname).toBe('/feed'));

act(() => navigation.current?.navigate('Settings'));
act(() => navigation.current?.navigate('Chat'));

expect(window.location.pathname).toBe('/chat');
await waitFor(() => expect(window.location.pathname).toBe('/chat'));

act(() => navigation.current?.navigate('Home'));

jest.runAllTimers();

expect(window.location.pathname).toBe('/edit');
await waitFor(() => expect(window.location.pathname).toBe('/edit'));
});
32 changes: 4 additions & 28 deletions packages/native/src/useLinking.tsx
Expand Up @@ -60,35 +60,11 @@ const findMatchingState = <T extends NavigationState>(
/**
* Run async function in series as it's called.
*/
const series = (cb: () => Promise<void>) => {
// Whether we're currently handling a callback
let handling = false;
let queue: (() => Promise<void>)[] = [];

const callback = async () => {
try {
if (handling) {
// If we're currently handling a previous event, wait before handling this one
// Add the callback to the beginning of the queue
queue.unshift(callback);
return;
}

handling = true;

await cb();
} finally {
handling = false;

if (queue.length) {
// If we have queued items, handle the last one
const last = queue.pop();

last?.();
}
}
export const series = (cb: () => Promise<void>) => {
let queue = Promise.resolve();
const callback = () => {
queue = queue.then(cb);
};

return callback;
};

Expand Down

0 comments on commit d8dc693

Please sign in to comment.