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.

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

suspend in render, not in reducers #56497

Merged
merged 42 commits into from
Nov 2, 2023
Merged

Conversation

ztanner
Copy link
Member

@ztanner ztanner commented Oct 5, 2023

This removes our current convention of throwing promises in reducers in favor of returning promises that can be consumed by use instead. This will help unblock some future improvements (batching, PPR)

Reducers that would typically throw a promise now return their promise. This gets maintained by a mutable queue (initialized in hydrate) to ensure actions are processed in-order. The queue is also responsible for mutating state and passing it as an input to subsequent actions.

This PR does not modify reducer behavior to keep changes minimal, but there's more cleanup that we can do in a follow-up PR to remove things that previously assumed reducers would be replayed.

(I recommend reviewing with whitespace turned off)

@ijjk
Copy link
Member

ijjk commented Oct 5, 2023

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js feat/remove-suspending-reducers Change
buildDuration 10.5s 10.5s N/A
buildDurationCached 6.1s 6.7s ⚠️ +627ms
nodeModulesSize 175 MB 175 MB ⚠️ +36.8 kB
nextStartRea..uration (ms) 398ms 396ms N/A
Client Bundles (main, webpack) Overall increase ⚠️
vercel/next.js canary vercel/next.js feat/remove-suspending-reducers Change
199-HASH.js gzip 30 kB 30.5 kB ⚠️ +499 B
3f784ff6-HASH.js gzip 53.2 kB 53.2 kB
494.HASH.js gzip 182 B 182 B
framework-HASH.js gzip 45.5 kB 45.5 kB
main-app-HASH.js gzip 254 B 251 B N/A
main-HASH.js gzip 33.1 kB 33.1 kB N/A
webpack-HASH.js gzip 1.75 kB 1.75 kB N/A
Overall change 129 kB 129 kB ⚠️ +499 B
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js feat/remove-suspending-reducers Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary vercel/next.js feat/remove-suspending-reducers Change
_app-HASH.js gzip 205 B 205 B
_error-HASH.js gzip 182 B 181 B N/A
amp-HASH.js gzip 505 B 507 B N/A
css-HASH.js gzip 322 B 323 B N/A
dynamic-HASH.js gzip 2.59 kB 2.59 kB N/A
edge-ssr-HASH.js gzip 258 B 259 B N/A
head-HASH.js gzip 348 B 347 B N/A
hooks-HASH.js gzip 369 B 368 B N/A
image-HASH.js gzip 4.38 kB 4.38 kB N/A
index-HASH.js gzip 256 B 256 B
link-HASH.js gzip 2.67 kB 2.7 kB N/A
routerDirect..HASH.js gzip 318 B 318 B
script-HASH.js gzip 384 B 383 B N/A
withRouter-HASH.js gzip 319 B 320 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 885 B 885 B
Client Build Manifests
vercel/next.js canary vercel/next.js feat/remove-suspending-reducers Change
_buildManifest.js gzip 484 B 484 B
Overall change 484 B 484 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js feat/remove-suspending-reducers Change
index.html gzip 527 B 529 B N/A
link.html gzip 540 B 541 B N/A
withRouter.html gzip 523 B 523 B
Overall change 523 B 523 B
Edge SSR bundle Size Overall increase ⚠️
vercel/next.js canary vercel/next.js feat/remove-suspending-reducers Change
edge-ssr.js gzip 96.1 kB 96.1 kB N/A
page.js gzip 140 kB 140 kB ⚠️ +312 B
Overall change 140 kB 140 kB ⚠️ +312 B
Middleware size
vercel/next.js canary vercel/next.js feat/remove-suspending-reducers Change
middleware-b..fest.js gzip 625 B 628 B N/A
middleware-r..fest.js gzip 148 B 151 B N/A
middleware.js gzip 23 kB 23 kB N/A
edge-runtime..pack.js gzip 1.92 kB 1.92 kB
Overall change 1.92 kB 1.92 kB
Diff details
Diff for page.js

Diff too large to display

Diff for link-HASH.js
@@ -1,7 +1,7 @@
 (self["webpackChunk_N_E"] = self["webpackChunk_N_E"] || []).push([
   [644],
   {
-    /***/ 1794: /***/ function (
+    /***/ 8959: /***/ function (
       __unused_webpack_module,
       __unused_webpack_exports,
       __webpack_require__
@@ -9,7 +9,7 @@
       (window.__NEXT_P = window.__NEXT_P || []).push([
         "/link",
         function () {
-          return __webpack_require__(1921);
+          return __webpack_require__(9877);
         },
       ]);
       if (false) {
@@ -18,7 +18,7 @@
       /***/
     },
 
-    /***/ 172: /***/ function (module, exports) {
+    /***/ 91: /***/ function (module, exports) {
       "use strict";
 
       Object.defineProperty(exports, "__esModule", {
@@ -57,6 +57,9 @@
         ACTION_SERVER_ACTION: function () {
           return ACTION_SERVER_ACTION;
         },
+        isThenable: function () {
+          return isThenable;
+        },
       });
       const ACTION_REFRESH = "refresh";
       const ACTION_NAVIGATE = "navigate";
@@ -71,6 +74,13 @@
         PrefetchKind["FULL"] = "full";
         PrefetchKind["TEMPORARY"] = "temporary";
       })(PrefetchKind || (PrefetchKind = {}));
+      function isThenable(value) {
+        return (
+          value &&
+          (typeof value === "object" || typeof value === "function") &&
+          typeof value.then === "function"
+        );
+      }
       if (
         (typeof exports.default === "function" ||
           (typeof exports.default === "object" && exports.default !== null)) &&
@@ -86,7 +96,7 @@
       /***/
     },
 
-    /***/ 4847: /***/ function (module, exports, __webpack_require__) {
+    /***/ 2976: /***/ function (module, exports, __webpack_require__) {
       "use strict";
 
       Object.defineProperty(exports, "__esModule", {
@@ -98,7 +108,7 @@
           return getDomainLocale;
         },
       });
-      const _normalizetrailingslash = __webpack_require__(2003);
+      const _normalizetrailingslash = __webpack_require__(9076);
       const basePath =
         /* unused pure expression or super */ null && (false || "");
       function getDomainLocale(path, locale, locales, domainLocales) {
@@ -122,7 +132,7 @@
       /***/
     },
 
-    /***/ 7414: /***/ function (module, exports, __webpack_require__) {
+    /***/ 106: /***/ function (module, exports, __webpack_require__) {
       "use strict";
       /* __next_internal_client_entry_do_not_use__  cjs */
       Object.defineProperty(exports, "__esModule", {
@@ -138,17 +148,17 @@
       const _react = /*#__PURE__*/ _interop_require_default._(
         __webpack_require__(959)
       );
-      const _resolvehref = __webpack_require__(279);
-      const _islocalurl = __webpack_require__(7377);
-      const _formaturl = __webpack_require__(9616);
-      const _utils = __webpack_require__(3869);
-      const _addlocale = __webpack_require__(5752);
-      const _routercontextsharedruntime = __webpack_require__(1840);
-      const _approutercontextsharedruntime = __webpack_require__(3057);
-      const _useintersection = __webpack_require__(9285);
-      const _getdomainlocale = __webpack_require__(4847);
-      const _addbasepath = __webpack_require__(5853);
-      const _routerreducertypes = __webpack_require__(172);
+      const _resolvehref = __webpack_require__(8873);
+      const _islocalurl = __webpack_require__(1023);
+      const _formaturl = __webpack_require__(3482);
+      const _utils = __webpack_require__(4597);
+      const _addlocale = __webpack_require__(6845);
+      const _routercontextsharedruntime = __webpack_require__(1049);
+      const _approutercontextsharedruntime = __webpack_require__(4095);
+      const _useintersection = __webpack_require__(4991);
+      const _getdomainlocale = __webpack_require__(2976);
+      const _addbasepath = __webpack_require__(7677);
+      const _routerreducertypes = __webpack_require__(91);
       const prefetched = new Set();
       function prefetch(router, href, as, options, appOptions, isAppRouter) {
         if (false) {
@@ -567,7 +577,7 @@
       /***/
     },
 
-    /***/ 9285: /***/ function (module, exports, __webpack_require__) {
+    /***/ 4991: /***/ function (module, exports, __webpack_require__) {
       "use strict";
 
       Object.defineProperty(exports, "__esModule", {
@@ -580,7 +590,7 @@
         },
       });
       const _react = __webpack_require__(959);
-      const _requestidlecallback = __webpack_require__(5781);
+      const _requestidlecallback = __webpack_require__(3136);
       const hasIntersectionObserver =
         typeof IntersectionObserver === "function";
       const observers = new Map();
@@ -693,7 +703,7 @@
       /***/
     },
 
-    /***/ 1921: /***/ function (
+    /***/ 9877: /***/ function (
       __unused_webpack_module,
       __webpack_exports__,
       __webpack_require__
@@ -709,7 +719,7 @@
       /* harmony import */ var react_jsx_runtime__WEBPACK_IMPORTED_MODULE_0__ =
         __webpack_require__(1527);
       /* harmony import */ var next_link__WEBPACK_IMPORTED_MODULE_1__ =
-        __webpack_require__(3639);
+        __webpack_require__(2075);
       /* harmony import */ var next_link__WEBPACK_IMPORTED_MODULE_1___default =
         /*#__PURE__*/ __webpack_require__.n(
           next_link__WEBPACK_IMPORTED_MODULE_1__
@@ -740,12 +750,12 @@
       /***/
     },
 
-    /***/ 3639: /***/ function (
+    /***/ 2075: /***/ function (
       module,
       __unused_webpack_exports,
       __webpack_require__
     ) {
-      module.exports = __webpack_require__(7414);
+      module.exports = __webpack_require__(106);
 
       /***/
     },
@@ -756,7 +766,7 @@
       return __webpack_require__((__webpack_require__.s = moduleId));
     };
     /******/ __webpack_require__.O(0, [774, 888, 179], function () {
-      return __webpack_exec__(1794);
+      return __webpack_exec__(8959);
     });
     /******/ var __webpack_exports__ = __webpack_require__.O();
     /******/ _N_E = __webpack_exports__;
Diff for 199-HASH.js

Diff too large to display

Commit: a41dbd7

@ijjk
Copy link
Member

ijjk commented Oct 5, 2023

Tests Passed

@ztanner ztanner force-pushed the feat/remove-suspending-reducers branch from 388f345 to d669590 Compare October 10, 2023 17:44
@ztanner ztanner force-pushed the feat/remove-suspending-reducers branch from 9b1aa76 to 3235239 Compare October 10, 2023 19:47
@ztanner ztanner force-pushed the feat/remove-suspending-reducers branch 2 times, most recently from b500df7 to 5ec4ca2 Compare October 11, 2023 03:45
@ztanner ztanner force-pushed the feat/remove-suspending-reducers branch from 5ec4ca2 to 9f47f6f Compare October 11, 2023 03:50
@ztanner ztanner force-pushed the feat/remove-suspending-reducers branch from e5f06b5 to 00660cf Compare October 20, 2023 04:18
@timneutkens timneutkens merged commit 85abc48 into canary Nov 2, 2023
57 checks passed
@timneutkens timneutkens deleted the feat/remove-suspending-reducers branch November 2, 2023 10:53
franky47 added a commit to franky47/next.js that referenced this pull request Nov 10, 2023
Between 14.0.2-canary.6 and 14.0.2-canary.7, a change was introduced
in vercel#56497 that turned the Redux store state into Promises,
rather than a synchronous state update.

This caused the `sync` function used to send state update to the Redux
Devtools to be recreated on every dispatch, which in turn, by referential
instability, caused the `HistoryUpdater` component to re-render and
trigger a `history.replaceState` with no particular change, but with the
internal `canonicalUrl`.

When an app does a soft/shallow navigation by calling history methods
directly (currently the only way to do shallow search params updates in
the app router), these changes would have been overwritten by any
prefetch (eg: hovering or mounting a Link), which is usually a no-op
for the navigation state.

This commit removes the `sync` function for the Redux devtools,
as it cannot function as-is: actual state updates need to be awaited
and forwarded somewhere else in the router if this behaviour is to
be maintained, and should be decoupled from history calls to prevent
side-effects.
franky47 added a commit to franky47/next.js that referenced this pull request Nov 10, 2023
Between 14.0.2-canary.6 and 14.0.2-canary.7, a change was introduced
in vercel#56497 that turned the Redux store state into Promises,
rather than a synchronous state update.

This caused the `sync` function used to send state update to the Redux
Devtools to be recreated on every dispatch, which in turn, by referential
instability, caused the `HistoryUpdater` component to re-render and
trigger a `history.replaceState` with no particular change, but with the
internal `canonicalUrl`.

When an app does a soft/shallow navigation by calling history methods
directly (currently the only way to do shallow search params updates in
the app router), these changes would have been overwritten by any
prefetch (eg: hovering or mounting a Link), which is usually a no-op
for the navigation state.

This commit removes the `sync` function for the Redux devtools,
as it cannot function as-is: actual state updates need to be awaited
and forwarded somewhere else in the router if this behaviour is to
be maintained, and should be decoupled from history calls to prevent
side-effects.
franky47 added a commit to franky47/next.js that referenced this pull request Nov 13, 2023
Between 14.0.2-canary.6 and 14.0.2-canary.7, a change was introduced
in vercel#56497 that turned the Redux store state into Promises,
rather than a synchronous state update.

This caused the `sync` function used to send state update to the Redux
Devtools to be recreated on every dispatch, which in turn, by referential
instability, caused the `HistoryUpdater` component to re-render and
trigger a `history.replaceState` with no particular change, but with the
internal `canonicalUrl`.

When an app does a soft/shallow navigation by calling history methods
directly (currently the only way to do shallow search params updates in
the app router), these changes would have been overwritten by any
prefetch (eg: hovering or mounting a Link), which is usually a no-op
for the navigation state.

This commit removes the `sync` function for the Redux devtools,
as it cannot function as-is: actual state updates need to be awaited
and forwarded somewhere else in the router if this behaviour is to
be maintained, and should be decoupled from history calls to prevent
side-effects.
franky47 added a commit to franky47/next.js that referenced this pull request Nov 13, 2023
Between 14.0.2-canary.6 and 14.0.2-canary.7, a change was introduced
in vercel#56497 that turned the Redux store state into Promises,
rather than a synchronous state update.

This caused the `sync` function used to send state update to the Redux
Devtools to be recreated on every dispatch, which in turn, by referential
instability, caused the `HistoryUpdater` component to re-render and
trigger a `history.replaceState` with no particular change, but with the
internal `canonicalUrl`.

When an app does a soft/shallow navigation by calling history methods
directly (currently the only way to do shallow search params updates in
the app router), these changes would have been overwritten by any
prefetch (eg: hovering or mounting a Link), which is usually a no-op
for the navigation state.

This commit removes the `sync` function for the Redux devtools,
as it cannot function as-is: actual state updates need to be awaited
and forwarded somewhere else in the router if this behaviour is to
be maintained, and should be decoupled from history calls to prevent
side-effects.
franky47 added a commit to franky47/next.js that referenced this pull request Nov 14, 2023
Between 14.0.2-canary.6 and 14.0.2-canary.7, a change was introduced
in vercel#56497 that turned the Redux store state into Promises,
rather than a synchronous state update.

This caused the `sync` function used to send state update to the Redux
Devtools to be recreated on every dispatch, which in turn, by referential
instability, caused the `HistoryUpdater` component to re-render and
trigger a `history.replaceState` with no particular change, but with the
internal `canonicalUrl`.

When an app does a soft/shallow navigation by calling history methods
directly (currently the only way to do shallow search params updates in
the app router), these changes would have been overwritten by any
prefetch (eg: hovering or mounting a Link), which is usually a no-op
for the navigation state.

This commit removes the `sync` function for the Redux devtools,
as it cannot function as-is: actual state updates need to be awaited
and forwarded somewhere else in the router if this behaviour is to
be maintained, and should be decoupled from history calls to prevent
side-effects.
franky47 added a commit to franky47/next.js that referenced this pull request Nov 14, 2023
Between 14.0.2-canary.6 and 14.0.2-canary.7, a change was introduced
in vercel#56497 that turned the Redux store state into Promises,
rather than a synchronous state update.

This caused the `sync` function used to send state update to the Redux
Devtools to be recreated on every dispatch, which in turn, by referential
instability, caused the `HistoryUpdater` component to re-render and
trigger a `history.replaceState` with no particular change, but with the
internal `canonicalUrl`.

When an app does a soft/shallow navigation by calling history methods
directly (currently the only way to do shallow search params updates in
the app router), these changes would have been overwritten by any
prefetch (eg: hovering or mounting a Link), which is usually a no-op
for the navigation state.

This commit removes the `sync` function for the Redux devtools,
as it cannot function as-is: actual state updates need to be awaited
and forwarded somewhere else in the router if this behaviour is to
be maintained, and should be decoupled from history calls to prevent
side-effects.
kodiakhq bot pushed a commit that referenced this pull request Nov 14, 2023
## Description

Between 14.0.2-canary.6 and 14.0.2-canary.7, a change was introduced in #56497 that turned the Redux store state into a Promise, rather than a synchronous state update.

This caused the `sync` function -- used to send state updates to the Redux Devtools -- to be recreated on every dispatch, which in turn, by referential instability, caused the `HistoryUpdater` component to re-render and trigger a `history.replaceState` with no particular change, but with the internal `canonicalUrl`.

When an app does a soft/shallow navigation by calling history methods directly (currently the only way to do shallow search params updates in the app router), these changes would have been overwritten by any prefetch (eg: hovering or mounting a Link), which is usually a no-op for the navigation state.

This PR changes the `sync` function to take the state as an argument rather than as a closure. The whole app router state is also unwrapped only once, and fed to the HistoryUpdater. Changes to its contents made by reducers will cause the HistoryUpdater effect to re-run, triggering history updates and a call to the sync function.

## Context

I maintain [`next-usequerystate`](https://github.com/47ng/next-usequerystate), which is used in the Vercel dashboard, and which is impacted by this change (see 
[#388](47ng/nuqs#388)).

## History

@timneutkens introduced the `sync` function and the whole Redux devtools reducer in #39866, with the note:

> a new hook useReducerWithReduxDevtools has been added, we'll probably want to put this behind a compile-time flag when the new router is marked stable but until then it's useful to have it enabled by default (only 
when you have Redux Devtools installed ofcourse). 

If a different direction is needed to keep sending `RENDER_SYNC` actions to Redux devtools, I'll be happy to rework this PR to move the `sync` function into the action queue.

## Changes

- [x] Added e2e test. Requires a `start` mode as prefetch links are disabled in development. Test was verified to fail from next@>=12.0.2-canary.7 without the fix.


Co-authored-by: Zack Tanner <1939140+ztanner@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants