Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Use action implementation at time of dispatch #29618

Merged
merged 1 commit into from
May 28, 2024
+67 −12
Merged

Fix: Use action implementation at time of dispatch #29618

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 46 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMForm-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1097,7 +1097,7 @@ describe('ReactDOMForm', () => {
});

// @gate enableAsyncActions
test('queues multiple actions and runs them in order', async () => {
test('useActionState: queues multiple actions and runs them in order', async () => {
let action;
function App() {
const [state, dispatch, isPending] = useActionState(
Expand Down Expand Up @@ -1128,6 +1128,51 @@ describe('ReactDOMForm', () => {
expect(container.textContent).toBe('D');
});

// @gate enableAsyncActions
test(
'useActionState: when calling a queued action, uses the implementation ' +
'that was current at the time it was dispatched, not the most recent one',
async () => {
let action;
function App({throwIfActionIsDispatched}) {
const [state, dispatch, isPending] = useActionState(async (s, a) => {
if (throwIfActionIsDispatched) {
throw new Error('Oops!');
}
return await getText(a);
}, 'Initial');
action = dispatch;
return <Text text={state + (isPending ? ' (pending)' : '')} />;
}

const root = ReactDOMClient.createRoot(container);
await act(() => root.render(<App throwIfActionIsDispatched={false} />));
assertLog(['Initial']);

// Dispatch two actions. The first one is async, so it forces the second
// one into an async queue.
await act(() => action('First action'));
assertLog(['Initial (pending)']);
// This action won't run until the first one finishes.
await act(() => action('Second action'));

// While the first action is still pending, update a prop. This causes the
// inline action implementation to change, but it should not affect the
// behavior of the action that is already queued.
await act(() => root.render(<App throwIfActionIsDispatched={true} />));
assertLog(['Initial (pending)']);

// Finish both of the actions.
await act(() => resolveText('First action'));
await act(() => resolveText('Second action'));
assertLog(['Second action']);

// Confirm that if we dispatch yet another action, it uses the updated
// action implementation.
await expect(act(() => action('Third action'))).rejects.toThrow('Oops!');
},
);

// @gate enableAsyncActions
test('useActionState: works if action is sync', async () => {
let increment;
Expand Down
32 changes: 21 additions & 11 deletions packages/react-reconciler/src/ReactFiberHooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1966,13 +1966,15 @@ type ActionStateQueue<S, P> = {
action: (Awaited<S>, P) => S,
// This is a circular linked list of pending action payloads. It incudes the
// action that is currently running.
pending: ActionStateQueueNode<P> | null,
pending: ActionStateQueueNode<S, P> | null,
};

type ActionStateQueueNode<P> = {
type ActionStateQueueNode<S, P> = {
payload: P,
// This is the action implementation at the time it was dispatched.
action: (Awaited<S>, P) => S,
// This is never null because it's part of a circular linked list.
next: ActionStateQueueNode<P>,
next: ActionStateQueueNode<S, P>,
};

function dispatchActionState<S, P>(
Expand All @@ -1989,8 +1991,9 @@ function dispatchActionState<S, P>(
if (last === null) {
// There are no pending actions; this is the first one. We can run
// it immediately.
const newLast: ActionStateQueueNode<P> = {
const newLast: ActionStateQueueNode<S, P> = {
payload,
action: actionQueue.action,
next: (null: any), // circular
};
newLast.next = actionQueue.pending = newLast;
Expand All @@ -1999,13 +2002,14 @@ function dispatchActionState<S, P>(
actionQueue,
(setPendingState: any),
(setState: any),
payload,
newLast,
);
} else {
// There's already an action running. Add to the queue.
const first = last.next;
const newLast: ActionStateQueueNode<P> = {
const newLast: ActionStateQueueNode<S, P> = {
payload,
action: actionQueue.action,
next: first,
};
actionQueue.pending = last.next = newLast;
Expand All @@ -2016,11 +2020,8 @@ function runActionStateAction<S, P>(
actionQueue: ActionStateQueue<S, P>,
setPendingState: boolean => void,
setState: Dispatch<S | Awaited<S>>,
payload: P,
node: ActionStateQueueNode<S, P>,
) {
const action = actionQueue.action;
const prevState = actionQueue.state;

// This is a fork of startTransition
const prevTransition = ReactSharedInternals.T;
const currentTransition: BatchConfigTransition = {};
Expand All @@ -2033,6 +2034,15 @@ function runActionStateAction<S, P>(
// This will be reverted automatically when all actions are finished.
setPendingState(true);

// `node.action` represents the action function at the time it was dispatched.
// If this action was queued, it might be stale, i.e. it's not necessarily the
// most current implementation of the action, stored on `actionQueue`. This is
// intentional. The conceptual model for queued actions is that they are
// queued in a remote worker; the dispatch happens immediately, only the
// execution is delayed.
const action = node.action;
const payload = node.payload;
const prevState = actionQueue.state;
try {
const returnValue = action(prevState, payload);
const onStartTransitionFinish = ReactSharedInternals.S;
Expand Down Expand Up @@ -2136,7 +2146,7 @@ function finishRunningActionStateAction<S, P>(
actionQueue,
(setPendingState: any),
(setState: any),
next.payload,
next,
);
}
}
Expand Down