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

Rework <Link> behavior (backwards compatible) #36436

Merged

Conversation

timneutkens
Copy link
Member

@timneutkens timneutkens commented Apr 25, 2022

Fixes #32233

⚠️ If you're looking at this PR please read the complete description including the part about incremental adoption.

TLDR:

Official support for <Link href="/about">About</Link> / <Link href="/about"><CustomComponent /></Link> / <Link href="/about"><strong>About</strong></Link> where <Link> always renders <a> without edge cases where it doesn’t render <a>. You'll no longer have to put an empty <a> in <Link> with this enabled.

You can opt-into this behavior by running the codemod and setting newNextLinkBehavior: true in the experimental option in next.config.js.

To run the codemod, run npx @next/codemod new-link in the root of your project.

Full context

Changes to <Link>

  • Added an legacyBehavior prop that defaults to true to preserve the defaults we have today, this will allow to run a codemod on existing codebases to move them to the version where legacyBehavior becomes false by default
  • When using the new behavior <Link> always renders an <a> instead of having React.cloneElement and passing props onto a child element
  • When using the new behavior props that can be passed to <a> can be passed to <Link>. Previously you could do something like <Link href="/somewhere"><a target="_blank">Download</a></Link> but with <Link> rendering <a> it now allows these props to be set on link. E.g. <Link href="/somewhere" target="_blank"></Link> / <Link href="/somewhere" className="link"></Link>

Incremental Adoption / Codemod

The main reason we haven't made these changes before is that it breaks pretty much all Next.js apps, which is why I've been hesitant to make this change in the past. I've spent a bunch of time figuring out what the right approach is to rolling this out and ended up with an approach that requires existing apps to run a codemod that automatically opts their <Link> usage into the old behavior in order to keep the app functioning.

This codemod will auto-fix the usage where possible. For example:

  • When you have <Link href="/about"><a>About</a></Link> it'll auto-fix to <Link href="/about">About</Link>
  • When you have <Link href="/about"><a onClick={() => console.log('clicked')}>About</a></Link> it'll auto-fix to <Link href="/about" onClick={() => console.log('clicked')}>About</Link>
  • For cases where auto-fixing can't be applied the legacyBehavior prop is added. When you have <Link href="/about"><Component /></Link> it'll transform to <Link href="/about" legacyBehavior><Component /></Link> so that your app keeps functioning using the old behavior for that particular link. It's then up to the dev to move that case out of the legacyBehavior prop.

This default will be changed in Next.js 13, it does not affect existing apps in Next.js 12 unless opted in via experimental.newNextLinkBehavior and running the codemod.

Some code samples of what changed:

const CustomComponent = () => <strong>Hello</strong>

// Legacy behavior: `<a>` has to be nested otherwise it's excluded

// Renders: <a href="/about">About</a>. `<a>` has to be nested.
<Link href="/about">
  <a>About</a>  
</Link>

// Renders: <strong onClick={nextLinkClickHandler}>Hello</strong>. No `<a>` is included.
<Link href="/about">
  <strong>Hello</strong>
</Link>


// Renders: <strong onClick={nextLinkClickHandler}>Hello</strong>. No `<a>` is included.
<Link href="/about">
  <CustomComponent />
</Link>

// --------------------------------------------------
// New behavior: `<Link>` always renders `<a>`

// Renders: <a href="/about">About</a>. `<a>` no longer has to be nested.
<Link href="/about">
  About
</Link>

// Renders: <a href="/about"><strong>Hello</strong></a>. `<a>` is included.
<Link href="/about">
  <strong>Hello</strong>
</Link>

// Renders: <a href="/about"><strong>Hello</strong></a>. `<a>` is included.
<Link href="/about">
  <CustomComponent />
</Link>

Feature

  • Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • Related issues linked using fixes #number
  • Integration tests added
  • Errors have helpful link attached, see contributing.md

- Add onClick / onMouseEnter on Link
- Always render `<Link>` with `<a>` (no more nesting `<a>`)
- Add `oldBehavior` prop to use the previous behavior - allows for migrating existing link usage. Defaults to `oldBehavior={true}` till the next major version
@ijjk

This comment was marked as outdated.

@ijjk
Copy link
Member

ijjk commented Apr 25, 2022

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary timneutkens/next.js add/that-link-change-we-always-wanted-to-do Change
buildDuration 17.7s 17.9s ⚠️ +134ms
buildDurationCached 6.9s 6.9s ⚠️ +6ms
nodeModulesSize 481 MB 481 MB ⚠️ +4.98 kB
Page Load Tests Overall increase ✓
vercel/next.js canary timneutkens/next.js add/that-link-change-we-always-wanted-to-do Change
/ failed reqs 0 0
/ total time (seconds) 3.89 3.884 -0.01
/ avg req/sec 642.69 643.64 +0.95
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.978 1.899 -0.08
/error-in-render avg req/sec 1263.65 1316.19 +52.54
Client Bundles (main, webpack)
vercel/next.js canary timneutkens/next.js add/that-link-change-we-always-wanted-to-do Change
925.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42 kB 42 kB
main-HASH.js gzip 28.6 kB 28.6 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 72.3 kB 72.3 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary timneutkens/next.js add/that-link-change-we-always-wanted-to-do Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages Overall increase ⚠️
vercel/next.js canary timneutkens/next.js add/that-link-change-we-always-wanted-to-do Change
_app-HASH.js gzip 1.36 kB 1.36 kB
_error-HASH.js gzip 192 B 192 B
amp-HASH.js gzip 309 B 309 B
css-HASH.js gzip 327 B 327 B
dynamic-HASH.js gzip 3.04 kB 3.04 kB
head-HASH.js gzip 351 B 351 B
hooks-HASH.js gzip 920 B 920 B
image-HASH.js gzip 5.73 kB 5.73 kB
index-HASH.js gzip 263 B 263 B
link-HASH.js gzip 2.36 kB 2.63 kB ⚠️ +265 B
routerDirect..HASH.js gzip 320 B 320 B
script-HASH.js gzip 392 B 392 B
withRouter-HASH.js gzip 319 B 319 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 16 kB 16.3 kB ⚠️ +265 B
Client Build Manifests
vercel/next.js canary timneutkens/next.js add/that-link-change-we-always-wanted-to-do Change
_buildManifest.js gzip 461 B 461 B
Overall change 461 B 461 B
Rendered Page Sizes
vercel/next.js canary timneutkens/next.js add/that-link-change-we-always-wanted-to-do Change
index.html gzip 531 B 531 B
link.html gzip 544 B 544 B
withRouter.html gzip 525 B 525 B
Overall change 1.6 kB 1.6 kB

Diffs

Diff for _buildManifest.js
@@ -13,7 +13,7 @@ self.__BUILD_MANIFEST = {
   "/head": ["static\u002Fchunks\u002Fpages\u002Fhead-96a5d6ed07cf5a83.js"],
   "/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-9dfe734f583d4926.js"],
   "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-4ebd299a1253d245.js"],
-  "/link": ["static\u002Fchunks\u002Fpages\u002Flink-c605640c895e01ab.js"],
+  "/link": ["static\u002Fchunks\u002Fpages\u002Flink-5b526b867abc83fc.js"],
   "/routerDirect": [
     "static\u002Fchunks\u002Fpages\u002FrouterDirect-98eb70bf22fb21da.js"
   ],
Diff for link-HASH.js
@@ -105,6 +105,34 @@
               default: obj
             };
       }
+      function _objectWithoutProperties(source, excluded) {
+        if (source == null) return {};
+        var target = _objectWithoutPropertiesLoose(source, excluded);
+        var key, i;
+        if (Object.getOwnPropertySymbols) {
+          var sourceSymbolKeys = Object.getOwnPropertySymbols(source);
+          for (i = 0; i < sourceSymbolKeys.length; i++) {
+            key = sourceSymbolKeys[i];
+            if (excluded.indexOf(key) >= 0) continue;
+            if (!Object.prototype.propertyIsEnumerable.call(source, key))
+              continue;
+            target[key] = source[key];
+          }
+        }
+        return target;
+      }
+      function _objectWithoutPropertiesLoose(source, excluded) {
+        if (source == null) return {};
+        var target = {};
+        var sourceKeys = Object.keys(source);
+        var key, i;
+        for (i = 0; i < sourceKeys.length; i++) {
+          key = sourceKeys[i];
+          if (excluded.indexOf(key) >= 0) continue;
+          target[key] = source[key];
+        }
+        return target;
+      }
       var prefetched = {};
       function prefetch(router, href, as, options) {
         if (false || !router) return;
@@ -164,6 +192,11 @@
         });
       }
       function Link(props) {
+        var _legacyBehavior = props.legacyBehavior,
+          legacyBehavior =
+            _legacyBehavior === void 0
+              ? Boolean(false) !== true
+              : _legacyBehavior;
         if (false) {
           var hasWarned,
             optionalProps,
@@ -172,48 +205,72 @@
             requiredPropsGuard,
             createPropError;
         }
-        var p = props.prefetch !== false;
+        var children;
+        var hrefProp = props.href,
+          asProp = props.as,
+          childrenProp = props.children,
+          prefetchProp = props.prefetch,
+          passHref = props.passHref,
+          replace = props.replace,
+          shallow = props.shallow,
+          scroll = props.scroll,
+          locale = props.locale,
+          onClick = props.onClick,
+          onMouseEnter = props.onMouseEnter,
+          restProps = _objectWithoutProperties(props, [
+            "href",
+            "as",
+            "children",
+            "prefetch",
+            "passHref",
+            "replace",
+            "shallow",
+            "scroll",
+            "locale",
+            "onClick",
+            "onMouseEnter"
+          ]);
+        children = childrenProp;
+        if (legacyBehavior && typeof children === "string") {
+          children = /*#__PURE__*/ _react.default.createElement(
+            "a",
+            null,
+            children
+          );
+        }
+        var p = prefetchProp !== false;
         var router = (0, _router1).useRouter();
         var ref2 = _react.default.useMemo(
             function() {
               var ref = _slicedToArray(
-                  (0, _router).resolveHref(router, props.href, true),
+                  (0, _router).resolveHref(router, hrefProp, true),
                   2
                 ),
                 resolvedHref = ref[0],
                 resolvedAs = ref[1];
               return {
                 href: resolvedHref,
-                as: props.as
-                  ? (0, _router).resolveHref(router, props.as)
+                as: asProp
+                  ? (0, _router).resolveHref(router, asProp)
                   : resolvedAs || resolvedHref
               };
             },
-            [router, props.href, props.as]
+            [router, hrefProp, asProp]
           ),
           href = ref2.href,
           as = ref2.as;
         var previousHref = _react.default.useRef(href);
         var previousAs = _react.default.useRef(as);
-        var children = props.children,
-          replace = props.replace,
-          shallow = props.shallow,
-          scroll = props.scroll,
-          locale = props.locale;
-        if (typeof children === "string") {
-          children = /*#__PURE__*/ _react.default.createElement(
-            "a",
-            null,
-            children
-          );
-        }
         // This will return the first child, if multiple are provided it will throw an error
         var child;
-        if (false) {
-        } else {
-          child = _react.default.Children.only(children);
+        if (legacyBehavior) {
+          if (false) {
+          } else {
+            child = _react.default.Children.only(children);
+          }
         }
-        var childRef = child && typeof child === "object" && child.ref;
+        var childRef =
+          legacyBehavior && child && typeof child === "object" && child.ref;
         var ref1 = _slicedToArray(
             (0, _useIntersection).useIntersection({
               rootMargin: "200px"
@@ -232,14 +289,14 @@
               previousHref.current = href;
             }
             setIntersectionRef(el);
-            if (childRef) {
+            if (legacyBehavior && childRef) {
               if (typeof childRef === "function") childRef(el);
               else if (typeof childRef === "object") {
                 childRef.current = el;
               }
             }
           },
-          [as, childRef, href, resetVisible, setIntersectionRef]
+          [as, childRef, href, resetVisible, setIntersectionRef, legacyBehavior]
         );
         _react.default.useEffect(
           function() {
@@ -262,7 +319,14 @@
           onClick: function(e) {
             if (false) {
             }
-            if (child.props && typeof child.props.onClick === "function") {
+            if (!legacyBehavior && typeof onClick === "function") {
+              onClick(e);
+            }
+            if (
+              legacyBehavior &&
+              child.props &&
+              typeof child.props.onClick === "function"
+            ) {
               child.props.onClick(e);
             }
             if (!e.defaultPrevented) {
@@ -277,22 +341,30 @@
                 locale
               );
             }
-          }
-        };
-        childProps.onMouseEnter = function(e) {
-          if (child.props && typeof child.props.onMouseEnter === "function") {
-            child.props.onMouseEnter(e);
-          }
-          if ((0, _router).isLocalURL(href)) {
-            prefetch(router, href, as, {
-              priority: true
-            });
+          },
+          onMouseEnter: function(e) {
+            if (!legacyBehavior && typeof onMouseEnter === "function") {
+              onMouseEnter(e);
+            }
+            if (
+              legacyBehavior &&
+              child.props &&
+              typeof child.props.onMouseEnter === "function"
+            ) {
+              child.props.onMouseEnter(e);
+            }
+            if ((0, _router).isLocalURL(href)) {
+              prefetch(router, href, as, {
+                priority: true
+              });
+            }
           }
         };
         // If child is an <a> tag and doesn't have a href attribute, or if the 'passHref' property is
         // defined, we specify the current 'href', so that repetition is not needed by the user
         if (
-          props.passHref ||
+          !legacyBehavior ||
+          passHref ||
           (child.type === "a" && !("href" in child.props))
         ) {
           var curLocale1 =
@@ -318,7 +390,13 @@
               )
             );
         }
-        return /*#__PURE__*/ _react.default.cloneElement(child, childProps);
+        return legacyBehavior
+          ? /*#__PURE__*/ _react.default.cloneElement(child, childProps)
+          : /*#__PURE__*/ _react.default.createElement(
+              "a",
+              Object.assign({}, restProps, childProps),
+              children
+            );
       }
       var _default = Link;
       exports["default"] = _default;
Diff for link.html
@@ -27,7 +27,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/pages/link-c605640c895e01ab.js"
+      src="/_next/static/chunks/pages/link-5b526b867abc83fc.js"
       defer=""
     ></script>
     <script src="/_next/static/BUILD_ID/_buildManifest.js" defer=""></script>

Default Build with SWC (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary timneutkens/next.js add/that-link-change-we-always-wanted-to-do Change
buildDuration 20.7s 20.5s -208ms
buildDurationCached 6.8s 7.4s ⚠️ +562ms
nodeModulesSize 481 MB 481 MB ⚠️ +4.98 kB
Page Load Tests Overall increase ✓
vercel/next.js canary timneutkens/next.js add/that-link-change-we-always-wanted-to-do Change
/ failed reqs 0 0
/ total time (seconds) 3.909 3.857 -0.05
/ avg req/sec 639.62 648.1 +8.48
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.899 1.911 ⚠️ +0.01
/error-in-render avg req/sec 1316.18 1308.2 ⚠️ -7.98
Client Bundles (main, webpack)
vercel/next.js canary timneutkens/next.js add/that-link-change-we-always-wanted-to-do Change
925.HASH.js gzip 178 B 178 B
framework-HASH.js gzip 42.2 kB 42.2 kB
main-HASH.js gzip 29.1 kB 29.1 kB
webpack-HASH.js gzip 1.45 kB 1.45 kB
Overall change 72.9 kB 72.9 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary timneutkens/next.js add/that-link-change-we-always-wanted-to-do Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages Overall increase ⚠️
vercel/next.js canary timneutkens/next.js add/that-link-change-we-always-wanted-to-do Change
_app-HASH.js gzip 1.35 kB 1.35 kB
_error-HASH.js gzip 179 B 179 B
amp-HASH.js gzip 313 B 313 B
css-HASH.js gzip 325 B 325 B
dynamic-HASH.js gzip 3.02 kB 3.02 kB
head-HASH.js gzip 351 B 351 B
hooks-HASH.js gzip 921 B 921 B
image-HASH.js gzip 5.78 kB 5.78 kB
index-HASH.js gzip 261 B 261 B
link-HASH.js gzip 2.44 kB 2.74 kB ⚠️ +305 B
routerDirect..HASH.js gzip 322 B 322 B
script-HASH.js gzip 393 B 393 B
withRouter-HASH.js gzip 317 B 317 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 16.1 kB 16.4 kB ⚠️ +305 B
Client Build Manifests
vercel/next.js canary timneutkens/next.js add/that-link-change-we-always-wanted-to-do Change
_buildManifest.js gzip 457 B 457 B
Overall change 457 B 457 B
Rendered Page Sizes Overall decrease ✓
vercel/next.js canary timneutkens/next.js add/that-link-change-we-always-wanted-to-do Change
index.html gzip 531 B 531 B
link.html gzip 544 B 542 B -2 B
withRouter.html gzip 526 B 526 B
Overall change 1.6 kB 1.6 kB -2 B

Diffs

Diff for _buildManifest.js
@@ -13,7 +13,7 @@ self.__BUILD_MANIFEST = {
   "/head": ["static\u002Fchunks\u002Fpages\u002Fhead-96a5d6ed07cf5a83.js"],
   "/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-9dfe734f583d4926.js"],
   "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-4ebd299a1253d245.js"],
-  "/link": ["static\u002Fchunks\u002Fpages\u002Flink-c605640c895e01ab.js"],
+  "/link": ["static\u002Fchunks\u002Fpages\u002Flink-5b526b867abc83fc.js"],
   "/routerDirect": [
     "static\u002Fchunks\u002Fpages\u002FrouterDirect-98eb70bf22fb21da.js"
   ],
Diff for link-HASH.js
@@ -105,6 +105,34 @@
               default: obj
             };
       }
+      function _objectWithoutProperties(source, excluded) {
+        if (source == null) return {};
+        var target = _objectWithoutPropertiesLoose(source, excluded);
+        var key, i;
+        if (Object.getOwnPropertySymbols) {
+          var sourceSymbolKeys = Object.getOwnPropertySymbols(source);
+          for (i = 0; i < sourceSymbolKeys.length; i++) {
+            key = sourceSymbolKeys[i];
+            if (excluded.indexOf(key) >= 0) continue;
+            if (!Object.prototype.propertyIsEnumerable.call(source, key))
+              continue;
+            target[key] = source[key];
+          }
+        }
+        return target;
+      }
+      function _objectWithoutPropertiesLoose(source, excluded) {
+        if (source == null) return {};
+        var target = {};
+        var sourceKeys = Object.keys(source);
+        var key, i;
+        for (i = 0; i < sourceKeys.length; i++) {
+          key = sourceKeys[i];
+          if (excluded.indexOf(key) >= 0) continue;
+          target[key] = source[key];
+        }
+        return target;
+      }
       var prefetched = {};
       function prefetch(router, href, as, options) {
         if (false || !router) return;
@@ -164,6 +192,11 @@
         });
       }
       function Link(props) {
+        var _legacyBehavior = props.legacyBehavior,
+          legacyBehavior =
+            _legacyBehavior === void 0
+              ? Boolean(false) !== true
+              : _legacyBehavior;
         if (false) {
           var hasWarned,
             optionalProps,
@@ -172,48 +205,72 @@
             requiredPropsGuard,
             createPropError;
         }
-        var p = props.prefetch !== false;
+        var children;
+        var hrefProp = props.href,
+          asProp = props.as,
+          childrenProp = props.children,
+          prefetchProp = props.prefetch,
+          passHref = props.passHref,
+          replace = props.replace,
+          shallow = props.shallow,
+          scroll = props.scroll,
+          locale = props.locale,
+          onClick = props.onClick,
+          onMouseEnter = props.onMouseEnter,
+          restProps = _objectWithoutProperties(props, [
+            "href",
+            "as",
+            "children",
+            "prefetch",
+            "passHref",
+            "replace",
+            "shallow",
+            "scroll",
+            "locale",
+            "onClick",
+            "onMouseEnter"
+          ]);
+        children = childrenProp;
+        if (legacyBehavior && typeof children === "string") {
+          children = /*#__PURE__*/ _react.default.createElement(
+            "a",
+            null,
+            children
+          );
+        }
+        var p = prefetchProp !== false;
         var router = (0, _router1).useRouter();
         var ref2 = _react.default.useMemo(
             function() {
               var ref = _slicedToArray(
-                  (0, _router).resolveHref(router, props.href, true),
+                  (0, _router).resolveHref(router, hrefProp, true),
                   2
                 ),
                 resolvedHref = ref[0],
                 resolvedAs = ref[1];
               return {
                 href: resolvedHref,
-                as: props.as
-                  ? (0, _router).resolveHref(router, props.as)
+                as: asProp
+                  ? (0, _router).resolveHref(router, asProp)
                   : resolvedAs || resolvedHref
               };
             },
-            [router, props.href, props.as]
+            [router, hrefProp, asProp]
           ),
           href = ref2.href,
           as = ref2.as;
         var previousHref = _react.default.useRef(href);
         var previousAs = _react.default.useRef(as);
-        var children = props.children,
-          replace = props.replace,
-          shallow = props.shallow,
-          scroll = props.scroll,
-          locale = props.locale;
-        if (typeof children === "string") {
-          children = /*#__PURE__*/ _react.default.createElement(
-            "a",
-            null,
-            children
-          );
-        }
         // This will return the first child, if multiple are provided it will throw an error
         var child;
-        if (false) {
-        } else {
-          child = _react.default.Children.only(children);
+        if (legacyBehavior) {
+          if (false) {
+          } else {
+            child = _react.default.Children.only(children);
+          }
         }
-        var childRef = child && typeof child === "object" && child.ref;
+        var childRef =
+          legacyBehavior && child && typeof child === "object" && child.ref;
         var ref1 = _slicedToArray(
             (0, _useIntersection).useIntersection({
               rootMargin: "200px"
@@ -232,14 +289,14 @@
               previousHref.current = href;
             }
             setIntersectionRef(el);
-            if (childRef) {
+            if (legacyBehavior && childRef) {
               if (typeof childRef === "function") childRef(el);
               else if (typeof childRef === "object") {
                 childRef.current = el;
               }
             }
           },
-          [as, childRef, href, resetVisible, setIntersectionRef]
+          [as, childRef, href, resetVisible, setIntersectionRef, legacyBehavior]
         );
         _react.default.useEffect(
           function() {
@@ -262,7 +319,14 @@
           onClick: function(e) {
             if (false) {
             }
-            if (child.props && typeof child.props.onClick === "function") {
+            if (!legacyBehavior && typeof onClick === "function") {
+              onClick(e);
+            }
+            if (
+              legacyBehavior &&
+              child.props &&
+              typeof child.props.onClick === "function"
+            ) {
               child.props.onClick(e);
             }
             if (!e.defaultPrevented) {
@@ -277,22 +341,30 @@
                 locale
               );
             }
-          }
-        };
-        childProps.onMouseEnter = function(e) {
-          if (child.props && typeof child.props.onMouseEnter === "function") {
-            child.props.onMouseEnter(e);
-          }
-          if ((0, _router).isLocalURL(href)) {
-            prefetch(router, href, as, {
-              priority: true
-            });
+          },
+          onMouseEnter: function(e) {
+            if (!legacyBehavior && typeof onMouseEnter === "function") {
+              onMouseEnter(e);
+            }
+            if (
+              legacyBehavior &&
+              child.props &&
+              typeof child.props.onMouseEnter === "function"
+            ) {
+              child.props.onMouseEnter(e);
+            }
+            if ((0, _router).isLocalURL(href)) {
+              prefetch(router, href, as, {
+                priority: true
+              });
+            }
           }
         };
         // If child is an <a> tag and doesn't have a href attribute, or if the 'passHref' property is
         // defined, we specify the current 'href', so that repetition is not needed by the user
         if (
-          props.passHref ||
+          !legacyBehavior ||
+          passHref ||
           (child.type === "a" && !("href" in child.props))
         ) {
           var curLocale1 =
@@ -318,7 +390,13 @@
               )
             );
         }
-        return /*#__PURE__*/ _react.default.cloneElement(child, childProps);
+        return legacyBehavior
+          ? /*#__PURE__*/ _react.default.cloneElement(child, childProps)
+          : /*#__PURE__*/ _react.default.createElement(
+              "a",
+              Object.assign({}, restProps, childProps),
+              children
+            );
       }
       var _default = Link;
       exports["default"] = _default;
Diff for link.html
@@ -27,7 +27,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/pages/link-c605640c895e01ab.js"
+      src="/_next/static/chunks/pages/link-5b526b867abc83fc.js"
       defer=""
     ></script>
     <script src="/_next/static/BUILD_ID/_buildManifest.js" defer=""></script>
Commit: d558a91

@jacobrask
Copy link

Nice incremental approach! Would a name like autoForwardProps or similar be more self explanatory both in the component props and config options? What if there’s an even newer link behavior some time from now?

@timneutkens
Copy link
Member Author

Nice incremental approach! Would a name like autoForwardProps or similar be more self explanatory both in the component props and config options? What if there’s an even newer link behavior some time from now?

It's much more complicated than just forwarding the props, e.g. React.cloneElement is used and it doesn't actually forward props, it adds onClick onto the child component and you can't add props that <a> would generally have onto <Link> in today's <Link>.

The general idea of the prop is that oldBehavior is something you never manually add to your application's links, it's something the codemod adds for you that can then be cleaned up / fixed at a later point in time. This way your app does not get stuck on an older version just because you built it on Next.js 12 and below.

@BButner
Copy link

BButner commented Apr 25, 2022

Are there plans to move className into Link, for easier styling? That way we wouldn't have to wrap the child text in any other element, and the styling would be passed from Link to the a tag?

@timneutkens
Copy link
Member Author

timneutkens commented Apr 25, 2022

Are there plans to move className into Link, for easier styling? That way we wouldn't have to wrap the child text in any other element, and the styling would be passed from Link to the a tag?

This is covered by this PR already and included in the tests: https://github.com/vercel/next.js/pull/36436/files#diff-e252bd01564b73f2c5d306ae70bb85f4b0022712b4e64225cc1a8f1e547469d7.

It is mentioned by the third item in changes to <Link> that all props <a> can take are supported on <Link>, this includes className. I've added an example using className to that item 👍

@BButner
Copy link

BButner commented Apr 25, 2022

Are there plans to move className into Link, for easier styling? That way we wouldn't have to wrap the child text in any other element, and the styling would be passed from Link to the a tag?

This is covered by this PR already and included in the tests: https://github.com/vercel/next.js/pull/36436/files#diff-e252bd01564b73f2c5d306ae70bb85f4b0022712b4e64225cc1a8f1e547469d7.

It is mentioned by the third item in changes to <Link> that all props <a> can take are supported on <Link>, this includes className. I've added an example using className to that item 👍

My apologies, I didn't read that thoroughly enough. This change is awesome ♥

@timneutkens
Copy link
Member Author

@BButner no worries, easy to miss, lot of content in this PR 👍

SuttonJack
SuttonJack previously approved these changes Apr 25, 2022
Copy link
Contributor

@SuttonJack SuttonJack left a comment

Choose a reason for hiding this comment

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

lgtm

packages/next/client/link.tsx Outdated Show resolved Hide resolved
packages/next/client/link.tsx Outdated Show resolved Hide resolved
packages/next/client/link.tsx Outdated Show resolved Hide resolved
@sammdec
Copy link

sammdec commented Apr 25, 2022

We use a custom a element created by stitches, how will this work with the css prop that stitches creates?

@kodiakhq kodiakhq bot merged commit 489e65e into vercel:canary Apr 25, 2022
@timneutkens timneutkens deleted the add/that-link-change-we-always-wanted-to-do branch April 26, 2022 07:12
@timneutkens
Copy link
Member Author

We use a custom a element created by stitches, how will this work with the css prop that stitches creates?

@sammdec it just works: https://github.com/vercel/next.js/pull/36474/files#diff-43f403de29e5b69987940b10d46c7409b7663f4a57fd16ee07ce9574e893b840R12-R16. This test is based on the existing example in the examples dir.

@sammdec
Copy link

sammdec commented Apr 26, 2022

@timneutkens and does link work for both internal links and external links e.g https://

@timneutkens
Copy link
Member Author

@timneutkens and does link work for both internal links and external links e.g https://

Yes, it has for a while now, can't remember which version exactly but somewhere around Next.js 9.2 I think.

@sammdec
Copy link

sammdec commented Apr 26, 2022

Woah totally missed that one! Time to get updating!

karlhorky added a commit to upleveled/eslint-config-upleveled that referenced this pull request May 2, 2022
@andrewscofield
Copy link

@timneutkens Just ran the codemod on a site and it worked 99% for me. The only issue I ran into was a couple spots where I had for example:

<Link href="/products">
  <a>
    <Image ... />
    <h1>{title}</h1>
  </a>
</Link>

The code mode transformed it into:

<Link href="/products">
    <Image ... />
    <h1>{title}</h1>
</Link>

Causing this error: https://nextjs.org/docs/messages/link-multiple-children

But other wise it worked perfect for me. Just thought I'd give you a heads up, I think this could be a common scenario.

@timneutkens
Copy link
Member Author

@andrewscofield that means you didn't enable the experimental flag for the new behavior with experimental.newNextLinkBehavior as it's a message that can't be run into in the new behavior:

// next.config.js
module.exports = {
  experimental: {
    newNextLinkBehavior: true
  }
}

@andrewscofield
Copy link

Ahh ok that fixed it. I must have been looking at an old commit or something because I had it setup as experimental.newLinkBehavior Thanks, I'm a big fan of this change!

@SukkaW
Copy link
Contributor

SukkaW commented May 12, 2022

FYI, here is my thought about how to migrate <Link href="/about"><Component /></Link> to new Next.js link behavior:

  • styled component:
- const Component = styled.a`color: red;`;
+ const Component = styled(Link)`color: red`;

// non-"standard" styled components, E.g. styletron.org:
- const Component = styled('a', { color: 'red' });
+ const Component = styled(Link, { color: 'red' });

- <Link href="/about"><Component /></Link>
+ <Component href="/about" />
  • custom component:
- const Component = forwardRef((props, ref) => <a  {...props} sx={{ color: 'red' }} ref={ref} />);
+ const Component = forwardRef((props, ref) => <Link  {...props} sx={{ color: 'red' }} ref={ref} />);

- <Link href="/about"><Component /></Link>
+ <Component href="/about" />
  • TBD

When Next.js 13 is finally released and legacyBehavior is removed, here is what Next.js should include in the migration guide.

@timneutkens
Copy link
Member Author

When Next.js 13 is finally released and legacyBehavior is removed, here is what Next.js should include in the migration guide.

legacyBehavior won't be removed in Next.js 13, it's part of the incremental adoption strategy. We'll want to provide how to update code that hit legacyBehavior through the codemod in the upgrade guide though 👍

@danielpl10
Copy link

Hi, from what version can I use this behavior? I'm on 12.1.6 and the only way this work for me was putting legacyBehavior={false} on the Link component

@stefanprobst
Copy link
Contributor

how would the following look like with the new behavior?:

import Link from '@mui/material/Link';
import NextLink from 'next/link';

<NextLink href="/" passHref>
  <Link>Home</Link>
</NextLink>

@robertwbradford
Copy link
Contributor

how would the following look like with the new behavior?:

import Link from '@mui/material/Link';
import NextLink from 'next/link';

<NextLink href="/" passHref>
  <Link>Home</Link>
</NextLink>

Would something like this work for the API?

import Link from '@mui/material/Link';
import NextLink from 'next/link';

<NextLink href="/" as={Link}>
  Home
</NextLink>

Where it renders using the Link component but attaches all the next/link behavior. as would default to "a".

@SukkaW
Copy link
Contributor

SukkaW commented Jun 3, 2022

how would the following look like with the new behavior?:

import Link from '@mui/material/Link';
import NextLink from 'next/link';

<NextLink href="/" passHref>
  <Link>Home</Link>
</NextLink>

Would something like this work for the API?

import Link from '@mui/material/Link';
import NextLink from 'next/link';

<NextLink href="/" as={Link}>
  Home
</NextLink>

Where it renders using the Link component but attaches all the next/link behavior. as would default to "a".

No. And here is the proper way of doing it:

import Link from '@mui/material/Link';
import NextLink from 'next/link';

<Link href="/" as={NextLink}>
  Home
</Link>

@blakeplumb
Copy link

blakeplumb commented Jun 29, 2022

So for all custom link components we want to use, we have to hope there is an as like property that will accept NextLink. Makes more sense to me to have NextLink handle this instead. @timneutkens Could you please help clarify this behavior as it's becoming a road block for my team in implementing this feature.

@christiankaindl
Copy link

If anyone is getting an error "Invalid transform choice" when running the codemod, then add the version to the npx command:

npx @next/codemod@12.2.0 new-link

Then it worked for me :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet