Remove mutable field from action types
#59221
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 |
acdlite
force-pushed
the
remove-mutable-field-from-action-types
branch
from
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 |
acdlite
requested review from
timneutkens,
ijjk,
shuding,
huozhi,
feedthejim,
ztanner and
wyattjoh
as code owners
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.
acdlite
force-pushed
the
remove-mutable-field-from-action-types
branch
from
fd6d3f0
to
cece1de
Compare
acdlite
commented
Dec 4, 2023
| @@ -276,172 +260,6 @@ describe('serverPatchReducer', () => { | |||
| `) | |||
| }) | |||
|
|
|||
| it('should apply server patch (concurrent)', async () => { | |||
There was a problem hiding this comment.
This was the same as the previous test except it ran the reducer twice. Since that no longer happens, I deleted it.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
mutablefield 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