-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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.
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
Remove mutable
field from action types
#59221
Remove mutable
field from action types
#59221
Conversation
Failing test suitesCommit: 969bf26
Expand output● serverPatchReducer › should apply server patch (concurrent)
Read more about building and testing Next.js in contributing.md. |
Tests Passed |
969bf26
to
fd6d3f0
Compare
Stats from current PRDefault BuildGeneral
Client Bundles (main, webpack)
Legacy Client Bundles (polyfills)
Client Pages
Client Build Manifests
Rendered Page Sizes
Edge SSR bundle Size
Middleware size
Next Runtimes
Diff detailsDiff for page.jsDiff too large to display Diff for 199-HASH.jsDiff too large to display |
Similar in spirit to vercel#58938. The app router reducer state used to be managed by useReducer, so it was written to be resilient to rebasing — the same action may be processed multiple times. Now that we've lifted the reducer outside of React (vercel#56497), each action runs only a single time. So we can simplify some of the logic. The purpose of the `mutable` field was so that if an action is processed multiple times, state could be reused between each run; for example, to prevent redundant network fetches. Now that this scenario can no longer happen, we can remove it. I had to update some of the unit tests in navigate-reducer because they were written with the assumption that the reducer was called multiple times. As far as I can tell, most of this behavior is covered by e2e tests anyway, so I don't think it's too risky.
fd6d3f0
to
cece1de
Compare
@@ -276,172 +260,6 @@ describe('serverPatchReducer', () => { | |||
`) | |||
}) | |||
|
|||
it('should apply server patch (concurrent)', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the same as the previous test except it ran the reducer twice. Since that no longer happens, I deleted it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
Similar in spirit to #58938.
The app router reducer state used to be managed by useReducer, so it was written to be resilient to rebasing — the same action may be processed multiple times. Now that we've lifted the reducer outside of React (#56497), each action runs only a single time. So we can simplify some of the logic.
The purpose of the
mutable
field was so that if an action is processed multiple times, state could be reused between each run; for example, to prevent redundant network fetches. Now that this scenario can no longer happen, we can remove it.I had to update some of the unit tests in navigate-reducer because they were written with the assumption that the reducer was called multiple times. As far as I can tell, most of this behavior is covered by e2e tests anyway, so I don't think it's too risky.
Closes NEXT-1782