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

Warn for unsafe next/head patterns #25065

Closed
wants to merge 16 commits into from

Conversation

devknoll
Copy link
Contributor

@devknoll devknoll commented May 12, 2021

next/head has a few pitfalls that'll be problematic in future versions of React and Next.js with e.g. suspense, streaming rendering, transitions, to name a few. Eventually, we'll probably want an API that doesn't have unsafe edge cases. In the meantime though, we want to give apps an opportunity to incrementally address any potential issues, by raising warnings during development.

The gist of it though, is that you should really only use <Head> for "cosmetic" tags: things like <title> or OpenGraph tags that don't really impact browser behavior. The reason being that non-cosmetic things are very likely to behave in unexpected ways with e.g. concurrent updates and transitions. Eventually, want to unlock streaming without having to wait for these cosmetic tags too. Additionally, because the order of promise resolution in suspense makes side effects non-deterministic, you should really only use <Head> once, typically at a leaf component that has enough context to set it.

If you really need to break these rules, there's nothing stopping you, but keep in mind you do so at your own risk and it might prevent you from using the latest features from Next.js and React.

@devknoll devknoll changed the title Warn for unsafe next/head patterns Warn for unsafe next/head patterns May 12, 2021
@ijjk

This comment has been minimized.

@devknoll devknoll marked this pull request as ready for review May 12, 2021 22:03
@@ -136,6 +153,11 @@ function reduceComponents(
.concat(defaultHead(props.inAmpMode))
.filter(unique())
.reverse()
.map((c) => {
shouldWarn =
shouldWarn || !SAFE_HEAD_CHECKS.map((fn) => fn(c)).includes(true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be always true if we have more than 1 element in head?
Suppose we add and <title> then this would always warn, even though its correct. Did I understand that right?

Copy link
Contributor Author

@devknoll devknoll May 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

headElements is (confusingly) the mounted instances of <Head>, whose props.children are the actual elements we want to include. So this is just checking if you have more than one <Head>. Adding another title wouldn't cause this to warn -- see the test cases 😄

@jonrh
Copy link

jonrh commented May 12, 2021

For reference these changes might fix #20924 and #19361. Maybe even more issues.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk
Copy link
Member

ijjk commented May 13, 2021

Failing test suites

Commit: 7127c01

test/acceptance/ReactRefreshLogBox.dev.test.js

  • server-side only compilation errors

Expand output

● server-side only compilation errors

ScriptTimeoutError: script timeout
  (Session info: headless chrome=90.0.4430.212)

  72 |
  73 |             // Wait for application to re-hydrate:
> 74 |             await browser.executeAsyncScript(function () {
     |             ^
  75 |               var callback = arguments[arguments.length - 1]
  76 |               if (window.__NEXT_HYDRATED) {
  77 |                 callback()

  at Object.throwDecodedError (../node_modules/selenium-webdriver/lib/error.js:550:15)
  at parseHttpResponse (../node_modules/selenium-webdriver/lib/http.js:565:13)
  at Executor.execute (../node_modules/selenium-webdriver/lib/http.js:491:26)
      at runMicrotasks (<anonymous>)
  at Proxy.execute (../node_modules/selenium-webdriver/lib/webdriver.js:700:17)
  at Object.patch (acceptance/helpers.js:74:13)
  at Object.<anonymous> (acceptance/ReactRefreshLogBox.dev.test.js:1387:3)

@ijjk
Copy link
Member

ijjk commented May 14, 2021

Stats from current PR

Default Server Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
buildDuration 14.2s 14.5s ⚠️ +298ms
buildDurationCached 4.4s 4.3s -96ms
nodeModulesSize 46.7 MB 46.7 MB ⚠️ +6.11 kB
Page Load Tests Overall decrease ⚠️
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
/ failed reqs 0 0
/ total time (seconds) 2.343 2.337 -0.01
/ avg req/sec 1066.99 1069.61 +2.62
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.224 1.25 ⚠️ +0.03
/error-in-render avg req/sec 2042.52 1999.8 ⚠️ -42.72
Client Bundles (main, webpack, commons)
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
framework-HASH.js gzip 39.3 kB 39.3 kB
main-HASH.js gzip 19.4 kB 19.4 kB
webpack-HASH.js gzip 994 B 994 B
Overall change 59.7 kB 59.7 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
polyfills-HASH.js gzip 31.1 kB 31.1 kB
Overall change 31.1 kB 31.1 kB
Client Pages Overall increase ⚠️
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
_app-HASH.js gzip 1.02 kB 1.02 kB
_error-HASH.js gzip 3.06 kB 3.13 kB ⚠️ +65 B
amp-HASH.js gzip 526 B 526 B
css-HASH.js gzip 334 B 334 B
hooks-HASH.js gzip 890 B 890 B
index-HASH.js gzip 262 B 262 B
link-HASH.js gzip 1.65 kB 1.65 kB
routerDirect..HASH.js gzip 331 B 331 B
withRouter-HASH.js gzip 329 B 329 B
bb14e60e810b..30f.css gzip 125 B 125 B
Overall change 8.54 kB 8.6 kB ⚠️ +65 B
Client Build Manifests Overall increase ⚠️
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
_buildManifest.js gzip 390 B 391 B ⚠️ +1 B
Overall change 390 B 391 B ⚠️ +1 B
Rendered Page Sizes
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
index.html gzip 560 B 560 B
link.html gzip 569 B 569 B
withRouter.html gzip 556 B 556 B
Overall change 1.69 kB 1.69 kB

Diffs

Diff for _buildManifest.js
@@ -2,7 +2,7 @@ self.__BUILD_MANIFEST = {
   __rewrites: { beforeFiles: [], afterFiles: [], fallback: [] },
   "/": ["static\u002Fchunks\u002Fpages\u002Findex-7b6faa8fb6d094524ece.js"],
   "/_error": [
-    "static\u002Fchunks\u002Fpages\u002F_error-6484fb7e0c6321ed99b4.js"
+    "static\u002Fchunks\u002Fpages\u002F_error-1f916a5376a96678bce2.js"
   ],
   "/amp": ["static\u002Fchunks\u002Fpages\u002Famp-cd6ecfee935256afbc83.js"],
   "/css": [
Diff for _error-HASH.js
@@ -293,24 +293,30 @@
         return newObj;
       }
 
+      var DEFAULT_CHARSET = /*#__PURE__*/ _react["default"].createElement(
+        "meta",
+        {
+          charSet: "utf-8"
+        }
+      );
+
+      var DEFAULT_VIEWPORT = /*#__PURE__*/ _react["default"].createElement(
+        "meta",
+        {
+          name: "viewport",
+          content: "width=device-width"
+        }
+      );
+
       function defaultHead() {
         var inAmpMode =
           arguments.length > 0 && arguments[0] !== undefined
             ? arguments[0]
             : false;
-        var head = [
-          /*#__PURE__*/ _react["default"].createElement("meta", {
-            charSet: "utf-8"
-          })
-        ];
+        var head = [DEFAULT_CHARSET];
 
         if (!inAmpMode) {
-          head.push(
-            /*#__PURE__*/ _react["default"].createElement("meta", {
-              name: "viewport",
-              content: "width=device-width"
-            })
-          );
+          head.push(DEFAULT_VIEWPORT);
         }
 
         return head;
@@ -420,7 +426,12 @@ Also adds support for deduplicated `key` properties
        */
 
       function reduceComponents(headElements, props) {
-        return headElements
+        var safetyViolations = new Set();
+
+        if (false) {
+        }
+
+        var components = headElements
           .reduce(function(list, headElement) {
             var headElementChildren = _react["default"].Children.toArray(
               headElement.props.children
@@ -433,6 +444,16 @@ Also adds support for deduplicated `key` properties
           .concat(defaultHead(props.inAmpMode))
           .filter(unique())
           .reverse()
+          .map(function(c) {
+            if (false) {
+              var _require,
+                renderToStaticMarkup,
+                isSafeNativeElement,
+                isCustomReactElement;
+            }
+
+            return c;
+          })
           .map(function(c, i) {
             var key = c.key || i;
 
@@ -464,6 +485,10 @@ Also adds support for deduplicated `key` properties
               key: key
             });
           });
+        return {
+          components: components,
+          safetyViolations: Array.from(safetyViolations)
+        };
       }
       /**
        * This component injects elements to `<head>` of your page.
@@ -564,20 +589,26 @@ Also adds support for deduplicated `key` properties
 
           _this = _super.call(this, props);
           _this._hasHeadManager = void 0;
+          _this._hasWarned = void 0;
 
           _this.emitChange = function() {
             if (_this._hasHeadManager) {
-              _this.props.headManager.updateHead(
-                _this.props.reduceComponentsToState(
-                  _toConsumableArray(_this.props.headManager.mountedInstances),
-                  _this.props
-                )
+              var state = _this.props.reduceComponentsToState(
+                _toConsumableArray(_this.props.headManager.mountedInstances),
+                _this.props
               );
+
+              if (false) {
+                var s;
+              }
+
+              _this.props.headManager.updateHead(state.components);
             }
           };
 
           _this._hasHeadManager =
             _this.props.headManager && _this.props.headManager.mountedInstances;
+          _this._hasWarned = false;
 
           if (isServer && _this._hasHeadManager) {
             _this.props.headManager.mountedInstances.add(

Serverless Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
buildDuration 17.6s 17.4s -209ms
buildDurationCached 6.4s 6.4s -16ms
nodeModulesSize 46.7 MB 46.7 MB ⚠️ +6.11 kB
Client Bundles (main, webpack, commons)
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
framework-HASH.js gzip 39.3 kB 39.3 kB
main-HASH.js gzip 19.4 kB 19.4 kB
webpack-HASH.js gzip 994 B 994 B
Overall change 59.7 kB 59.7 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
polyfills-HASH.js gzip 31.1 kB 31.1 kB
Overall change 31.1 kB 31.1 kB
Client Pages Overall increase ⚠️
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
_app-HASH.js gzip 1.02 kB 1.02 kB
_error-HASH.js gzip 3.06 kB 3.13 kB ⚠️ +65 B
amp-HASH.js gzip 526 B 526 B
css-HASH.js gzip 334 B 334 B
hooks-HASH.js gzip 890 B 890 B
index-HASH.js gzip 262 B 262 B
link-HASH.js gzip 1.65 kB 1.65 kB
routerDirect..HASH.js gzip 331 B 331 B
withRouter-HASH.js gzip 329 B 329 B
bb14e60e810b..30f.css gzip 125 B 125 B
Overall change 8.54 kB 8.6 kB ⚠️ +65 B
Client Build Manifests Overall increase ⚠️
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
_buildManifest.js gzip 390 B 391 B ⚠️ +1 B
Overall change 390 B 391 B ⚠️ +1 B
Serverless bundles Overall increase ⚠️
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
_error.js 1.34 MB 1.34 MB ⚠️ +392 B
404.html 2.42 kB 2.42 kB
500.html 2.41 kB 2.41 kB
amp.amp.html 10.8 kB 10.8 kB
amp.html 1.61 kB 1.61 kB
css.html 1.79 kB 1.79 kB
hooks.html 1.67 kB 1.67 kB
index.js 1.34 MB 1.35 MB ⚠️ +394 B
link.js 1.4 MB 1.4 MB ⚠️ +390 B
routerDirect.js 1.4 MB 1.4 MB ⚠️ +392 B
withRouter.js 1.4 MB 1.4 MB ⚠️ +390 B
Overall change 6.91 MB 6.91 MB ⚠️ +1.96 kB

Webpack 4 Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
buildDuration 12.3s 12.5s ⚠️ +242ms
buildDurationCached 5.4s 5.2s -250ms
nodeModulesSize 46.7 MB 46.7 MB ⚠️ +6.11 kB
Page Load Tests Overall increase ✓
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
/ failed reqs 0 0
/ total time (seconds) 2.444 2.367 -0.08
/ avg req/sec 1023.11 1056.22 +33.11
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.226 1.234 ⚠️ +0.01
/error-in-render avg req/sec 2039.73 2025.97 ⚠️ -13.76
Client Bundles (main, webpack, commons)
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
677f882d2ed8..HASH.js gzip 13.3 kB 13.3 kB
framework.HASH.js gzip 39 kB 39 kB
main-HASH.js gzip 7.26 kB 7.26 kB
webpack-HASH.js gzip 751 B 751 B
Overall change 60.3 kB 60.3 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
polyfills-HASH.js gzip 31.3 kB 31.3 kB
Overall change 31.3 kB 31.3 kB
Client Pages Overall increase ⚠️
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
_app-HASH.js gzip 1.28 kB 1.28 kB
_error-HASH.js gzip 3.74 kB 3.79 kB ⚠️ +56 B
amp-HASH.js gzip 536 B 536 B
css-HASH.js gzip 339 B 339 B
hooks-HASH.js gzip 887 B 887 B
index-HASH.js gzip 227 B 227 B
link-HASH.js gzip 1.64 kB 1.64 kB
routerDirect..HASH.js gzip 303 B 303 B
withRouter-HASH.js gzip 302 B 302 B
e025d2764813..52f.css gzip 125 B 125 B
Overall change 9.38 kB 9.44 kB ⚠️ +56 B
Client Build Manifests
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
_buildManifest.js gzip 420 B 420 B
Overall change 420 B 420 B
Rendered Page Sizes
vercel/next.js canary azukaru/next.js x-modern-next-head-new Change
index.html gzip 613 B 613 B
link.html gzip 619 B 619 B
withRouter.html gzip 606 B 606 B
Overall change 1.84 kB 1.84 kB

Diffs

Diff for _buildManifest.js
@@ -2,7 +2,7 @@ self.__BUILD_MANIFEST = {
   __rewrites: { beforeFiles: [], afterFiles: [], fallback: [] },
   "/": ["static\u002Fchunks\u002Fpages\u002Findex-b460df3d63326fbb06a1.js"],
   "/_error": [
-    "static\u002Fchunks\u002Fpages\u002F_error-51845a36fe9902c725c2.js"
+    "static\u002Fchunks\u002Fpages\u002F_error-216072a800de1b6af8d2.js"
   ],
   "/amp": ["static\u002Fchunks\u002Fpages\u002Famp-27f75ad11120c5cdedd1.js"],
   "/css": [
Diff for _error-HASH.js
@@ -397,24 +397,30 @@ _N_E = (window["webpackJsonp_N_E"] = window["webpackJsonp_N_E"] || []).push([
         return newObj;
       }
 
+      var DEFAULT_CHARSET = /*#__PURE__*/ _react["default"].createElement(
+        "meta",
+        {
+          charSet: "utf-8"
+        }
+      );
+
+      var DEFAULT_VIEWPORT = /*#__PURE__*/ _react["default"].createElement(
+        "meta",
+        {
+          name: "viewport",
+          content: "width=device-width"
+        }
+      );
+
       function defaultHead() {
         var inAmpMode =
           arguments.length > 0 && arguments[0] !== undefined
             ? arguments[0]
             : false;
-        var head = [
-          /*#__PURE__*/ _react["default"].createElement("meta", {
-            charSet: "utf-8"
-          })
-        ];
+        var head = [DEFAULT_CHARSET];
 
         if (!inAmpMode) {
-          head.push(
-            /*#__PURE__*/ _react["default"].createElement("meta", {
-              name: "viewport",
-              content: "width=device-width"
-            })
-          );
+          head.push(DEFAULT_VIEWPORT);
         }
 
         return head;
@@ -524,7 +530,12 @@ Also adds support for deduplicated `key` properties
        */
 
       function reduceComponents(headElements, props) {
-        return headElements
+        var safetyViolations = new Set();
+
+        if (false) {
+        }
+
+        var components = headElements
           .reduce(function(list, headElement) {
             var headElementChildren = _react["default"].Children.toArray(
               headElement.props.children
@@ -537,6 +548,16 @@ Also adds support for deduplicated `key` properties
           .concat(defaultHead(props.inAmpMode))
           .filter(unique())
           .reverse()
+          .map(function(c) {
+            if (false) {
+              var _require,
+                renderToStaticMarkup,
+                isSafeNativeElement,
+                isCustomReactElement;
+            }
+
+            return c;
+          })
           .map(function(c, i) {
             var key = c.key || i;
 
@@ -568,6 +589,10 @@ Also adds support for deduplicated `key` properties
               key: key
             });
           });
+        return {
+          components: components,
+          safetyViolations: Array.from(safetyViolations)
+        };
       }
       /**
        * This component injects elements to `<head>` of your page.
@@ -860,20 +885,26 @@ Also adds support for deduplicated `key` properties
 
           _this = _super.call(this, props);
           _this._hasHeadManager = void 0;
+          _this._hasWarned = void 0;
 
           _this.emitChange = function() {
             if (_this._hasHeadManager) {
-              _this.props.headManager.updateHead(
-                _this.props.reduceComponentsToState(
-                  _toConsumableArray(_this.props.headManager.mountedInstances),
-                  _this.props
-                )
+              var state = _this.props.reduceComponentsToState(
+                _toConsumableArray(_this.props.headManager.mountedInstances),
+                _this.props
               );
+
+              if (false) {
+                var s;
+              }
+
+              _this.props.headManager.updateHead(state.components);
             }
           };
 
           _this._hasHeadManager =
             _this.props.headManager && _this.props.headManager.mountedInstances;
+          _this._hasWarned = false;
 
           if (isServer && _this._hasHeadManager) {
             _this.props.headManager.mountedInstances.add(
Commit: e77c4a2

@shuding
Copy link
Member

shuding commented Jul 14, 2021

It’s still a very common need to put <script>s and <link>s inside <Head>. I think if they’re under the very top level (not inside a suspense boundary), it’s a safe case and we should probably not show the warning.

@ijjk
Copy link
Member

ijjk commented Jul 20, 2021

@devknoll is this PR still in progress or should we close for now?

@devknoll devknoll closed this Jul 20, 2021
@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
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.

5 participants