Skip to content

Commit 2ec6f21

Browse files
karlsanderosdnk
authored andcommitted
fix: replace series function used to queue async callbacks (#11485)
**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 <[email protected]>
1 parent 0b74a0c commit 2ec6f21

File tree

2 files changed

+15
-48
lines changed

2 files changed

+15
-48
lines changed

Diff for: packages/native/src/__tests__/NavigationContainer.test.tsx

+11-20
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import {
66
TabRouter,
77
useNavigationBuilder,
88
} from '@react-navigation/core';
9-
import { act, render } from '@testing-library/react-native';
9+
import { act, render, waitFor } from '@testing-library/react-native';
1010
import * as React from 'react';
1111

1212
import window from '../__mocks__/window';
@@ -18,9 +18,7 @@ Object.assign(global, window);
1818
// eslint-disable-next-line import/extensions
1919
jest.mock('../useLinking', () => require('../useLinking.tsx').default);
2020

21-
it('integrates with the history API', () => {
22-
jest.useFakeTimers();
23-
21+
it('integrates with the history API', async () => {
2422
const createStackNavigator = createNavigatorFactory((props: any) => {
2523
const { state, descriptors, NavigationContent } = useNavigationBuilder(
2624
StackRouter,
@@ -104,51 +102,44 @@ it('integrates with the history API', () => {
104102

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

107-
expect(window.location.pathname).toBe('/jane');
105+
await waitFor(() => expect(window.location.pathname).toBe('/jane'));
108106

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

111-
expect(window.location.pathname).toBe('/updates');
109+
await waitFor(() => expect(window.location.pathname).toBe('/updates'));
112110

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

115-
jest.runAllTimers();
116-
117-
expect(window.location.pathname).toBe('/jane');
113+
await waitFor(() => expect(window.location.pathname).toBe('/jane'));
118114

119115
act(() => {
120116
window.history.back();
121-
jest.runAllTimers();
122117
});
123118

124-
expect(window.location.pathname).toBe('/feed');
119+
await waitFor(() => expect(window.location.pathname).toBe('/feed'));
125120

126121
act(() => {
127122
window.history.forward();
128-
jest.runAllTimers();
129123
});
130124

131-
expect(window.location.pathname).toBe('/jane');
125+
await waitFor(() => expect(window.location.pathname).toBe('/jane'));
132126

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

135-
expect(window.location.pathname).toBe('/edit');
129+
await waitFor(() => expect(window.location.pathname).toBe('/edit'));
136130

137131
act(() => {
138132
window.history.go(-2);
139-
jest.runAllTimers();
140133
});
141134

142-
expect(window.location.pathname).toBe('/feed');
135+
await waitFor(() => expect(window.location.pathname).toBe('/feed'));
143136

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

147-
expect(window.location.pathname).toBe('/chat');
140+
await waitFor(() => expect(window.location.pathname).toBe('/chat'));
148141

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

151-
jest.runAllTimers();
152-
153-
expect(window.location.pathname).toBe('/edit');
144+
await waitFor(() => expect(window.location.pathname).toBe('/edit'));
154145
});

Diff for: packages/native/src/useLinking.tsx

+4-28
Original file line numberDiff line numberDiff line change
@@ -59,35 +59,11 @@ const findMatchingState = <T extends NavigationState>(
5959
/**
6060
* Run async function in series as it's called.
6161
*/
62-
const series = (cb: () => Promise<void>) => {
63-
// Whether we're currently handling a callback
64-
let handling = false;
65-
let queue: (() => Promise<void>)[] = [];
66-
67-
const callback = async () => {
68-
try {
69-
if (handling) {
70-
// If we're currently handling a previous event, wait before handling this one
71-
// Add the callback to the beginning of the queue
72-
queue.unshift(callback);
73-
return;
74-
}
75-
76-
handling = true;
77-
78-
await cb();
79-
} finally {
80-
handling = false;
81-
82-
if (queue.length) {
83-
// If we have queued items, handle the last one
84-
const last = queue.pop();
85-
86-
last?.();
87-
}
88-
}
62+
export const series = (cb: () => Promise<void>) => {
63+
let queue = Promise.resolve();
64+
const callback = () => {
65+
queue = queue.then(cb);
8966
};
90-
9167
return callback;
9268
};
9369

0 commit comments

Comments
 (0)