fix: `normalize-asset-prefix` adding leading slash when URL `assetPrefix` is provided #68518

devjiwonchoi · 2024-08-05T04:32:54Z

Why?

URL validation of normalizeAssetPrefix was wrong as it checks if is starting with ://:

next.js/packages/next/src/shared/lib/normalized-asset-prefix.ts

Lines 4 to 7 in 37df9ab

    
           // assetPrefix as a url 
        
           if (escapedAssetPrefix && escapedAssetPrefix.startsWith('://')) { 
        
             return escapedAssetPrefix.split('://', 2)[1] 
        
           }

This resulted the value to be /https://example.com instead of example.com.

x-ref: #68518 (comment)

How?

As normalizeAssetPrefix function was added for getSocketUrl, it removes the protocol of the URL.
But for reusability, this could be a friction to other callers.
Therefore we validate the URL and let the callers handle the URL (e.g. protocol).

devjiwonchoi · 2024-08-05T04:33:09Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @devjiwonchoi and the rest of your teammates on Graphite

ijjk · 2024-08-05T04:35:37Z

Tests Passed

ijjk · 2024-08-05T04:44:41Z

Stats from current PR

Default Build (Increase detected ⚠️)

General

	vercel/next.js canary	vercel/next.js 08-05-fix_normalize_asset_prefix_for_socket_if_start_with_http	Change
buildDuration	16.9s	17.6s	⚠️ +669ms
buildDurationCached	8.5s	7.2s	N/A
nodeModulesSize	352 MB	352 MB	N/A
nextStartRea..uration (ms)	427ms	420ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	vercel/next.js 08-05-fix_normalize_asset_prefix_for_socket_if_start_with_http	Change
124-HASH.js gzip	37.3 kB	37.3 kB	N/A
5121a57b-HASH.js gzip	51.9 kB	51.9 kB	N/A
7480.HASH.js gzip	169 B	169 B	✓
935-HASH.js gzip	5.19 kB	5.19 kB	N/A
framework-HASH.js gzip	56.7 kB	56.7 kB	✓
main-app-HASH.js gzip	224 B	225 B	N/A
main-HASH.js gzip	32.1 kB	32.2 kB	N/A
webpack-HASH.js gzip	1.71 kB	1.71 kB	✓
Overall change	58.6 kB	58.6 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	vercel/next.js 08-05-fix_normalize_asset_prefix_for_socket_if_start_with_http	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	vercel/next.js 08-05-fix_normalize_asset_prefix_for_socket_if_start_with_http	Change
_app-HASH.js gzip	193 B	194 B	N/A
_error-HASH.js gzip	193 B	192 B	N/A
amp-HASH.js gzip	509 B	511 B	N/A
css-HASH.js gzip	342 B	343 B	N/A
dynamic-HASH.js gzip	1.84 kB	1.84 kB	✓
edge-ssr-HASH.js gzip	265 B	266 B	N/A
head-HASH.js gzip	362 B	364 B	N/A
hooks-HASH.js gzip	393 B	393 B	✓
image-HASH.js gzip	4.4 kB	4.4 kB	✓
index-HASH.js gzip	268 B	267 B	N/A
link-HASH.js gzip	2.81 kB	2.81 kB	N/A
routerDirect..HASH.js gzip	329 B	327 B	N/A
script-HASH.js gzip	395 B	396 B	N/A
withRouter-HASH.js gzip	325 B	323 B	N/A
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	6.74 kB	6.74 kB	✓

Client Build Manifests

	vercel/next.js canary	vercel/next.js 08-05-fix_normalize_asset_prefix_for_socket_if_start_with_http	Change
_buildManifest.js gzip	748 B	748 B	✓
Overall change	748 B	748 B	✓

Rendered Page Sizes

	vercel/next.js canary	vercel/next.js 08-05-fix_normalize_asset_prefix_for_socket_if_start_with_http	Change
index.html gzip	522 B	521 B	N/A
link.html gzip	537 B	538 B	N/A
withRouter.html gzip	517 B	517 B	✓
Overall change	517 B	517 B	✓

Edge SSR bundle Size

	vercel/next.js canary	vercel/next.js 08-05-fix_normalize_asset_prefix_for_socket_if_start_with_http	Change
edge-ssr.js gzip	127 kB	127 kB	N/A
page.js gzip	168 kB	168 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	vercel/next.js 08-05-fix_normalize_asset_prefix_for_socket_if_start_with_http	Change
middleware-b..fest.js gzip	666 B	667 B	N/A
middleware-r..fest.js gzip	155 B	156 B	N/A
middleware.js gzip	29.6 kB	29.6 kB	N/A
edge-runtime..pack.js gzip	1.03 kB	1.03 kB	✓
Overall change	1.03 kB	1.03 kB	✓

Next Runtimes

	vercel/next.js canary	vercel/next.js 08-05-fix_normalize_asset_prefix_for_socket_if_start_with_http	Change
928-experime...dev.js gzip	310 B	310 B	✓
928.runtime.dev.js gzip	301 B	301 B	✓
app-page-exp...dev.js gzip	245 kB	246 kB	N/A
app-page-exp..prod.js gzip	120 kB	120 kB	✓
app-page-tur..prod.js gzip	132 kB	132 kB	✓
app-page-tur..prod.js gzip	128 kB	128 kB	✓
app-page.run...dev.js gzip	232 kB	232 kB	N/A
app-page.run..prod.js gzip	117 kB	117 kB	✓
app-route-ex...dev.js gzip	23.1 kB	23.1 kB	✓
app-route-ex..prod.js gzip	18.8 kB	18.8 kB	✓
app-route-tu..prod.js gzip	18.8 kB	18.8 kB	✓
app-route-tu..prod.js gzip	18.6 kB	18.6 kB	✓
app-route.ru...dev.js gzip	24.4 kB	24.4 kB	✓
app-route.ru..prod.js gzip	18.6 kB	18.6 kB	✓
pages-api-tu..prod.js gzip	9.6 kB	9.6 kB	✓
pages-api.ru...dev.js gzip	9.87 kB	9.87 kB	✓
pages-api.ru..prod.js gzip	9.59 kB	9.59 kB	✓
pages-turbo...prod.js gzip	21.6 kB	21.6 kB	✓
pages.runtim...dev.js gzip	22.2 kB	22.2 kB	✓
pages.runtim..prod.js gzip	21.6 kB	21.6 kB	✓
server.runti..prod.js gzip	56.8 kB	56.8 kB	✓
Overall change	772 kB	772 kB	✓

build cache Overall increase ⚠️

	vercel/next.js canary	vercel/next.js 08-05-fix_normalize_asset_prefix_for_socket_if_start_with_http	Change
0.pack gzip	1.41 MB	1.41 MB	⚠️ +1.51 kB
index.pack gzip	122 kB	120 kB	N/A
Overall change	1.41 MB	1.41 MB	⚠️ +1.51 kB

Diff details

Diff for page.js

Diff too large to display

Diff for middleware.js

Diff too large to display

Diff for edge-ssr.js

Diff too large to display

Diff for image-HASH.js

@@ -1,7 +1,7 @@
 (self["webpackChunk_N_E"] = self["webpackChunk_N_E"] || []).push([
   [8358],
   {
-    /***/ 8316: /***/ (
+    /***/ 8834: /***/ (
       __unused_webpack_module,
       __unused_webpack_exports,
       __webpack_require__
@@ -9,7 +9,7 @@
       (window.__NEXT_P = window.__NEXT_P || []).push([
         "/image",
         function () {
-          return __webpack_require__(4765);
+          return __webpack_require__(5780);
         },
       ]);
       if (false) {
@@ -18,7 +18,7 @@
       /***/
     },
 
-    /***/ 1112: /***/ (module, exports, __webpack_require__) => {
+    /***/ 7481: /***/ (module, exports, __webpack_require__) => {
       "use strict";
       /* __next_internal_client_entry_do_not_use__  cjs */
       Object.defineProperty(exports, "__esModule", {
@@ -40,17 +40,17 @@
         __webpack_require__(5596)
       );
       const _head = /*#__PURE__*/ _interop_require_default._(
-        __webpack_require__(9382)
+        __webpack_require__(9639)
       );
-      const _getimgprops = __webpack_require__(2097);
-      const _imageconfig = __webpack_require__(8714);
-      const _imageconfigcontextsharedruntime = __webpack_require__(1327);
-      const _warnonce = __webpack_require__(6055);
-      const _routercontextsharedruntime = __webpack_require__(8275);
+      const _getimgprops = __webpack_require__(1607);
+      const _imageconfig = __webpack_require__(2867);
+      const _imageconfigcontextsharedruntime = __webpack_require__(1015);
+      const _warnonce = __webpack_require__(6694);
+      const _routercontextsharedruntime = __webpack_require__(9562);
       const _imageloader = /*#__PURE__*/ _interop_require_default._(
-        __webpack_require__(6067)
+        __webpack_require__(3463)
       );
-      const _usemergedref = __webpack_require__(1261);
+      const _usemergedref = __webpack_require__(1057);
       // This is replaced by webpack define plugin
       const configEnv = {
         deviceSizes: [640, 750, 828, 1080, 1200, 1920, 2048, 3840],
@@ -371,7 +371,7 @@
       /***/
     },
 
-    /***/ 1261: /***/ (module, exports, __webpack_require__) => {
+    /***/ 1057: /***/ (module, exports, __webpack_require__) => {
       "use strict";
 
       Object.defineProperty(exports, "__esModule", {
@@ -440,7 +440,7 @@
       /***/
     },
 
-    /***/ 2097: /***/ (
+    /***/ 1607: /***/ (
       __unused_webpack_module,
       exports,
       __webpack_require__
@@ -456,9 +456,9 @@
           return getImgProps;
         },
       });
-      const _warnonce = __webpack_require__(6055);
-      const _imageblursvg = __webpack_require__(9254);
-      const _imageconfig = __webpack_require__(8714);
+      const _warnonce = __webpack_require__(6694);
+      const _imageblursvg = __webpack_require__(9866);
+      const _imageconfig = __webpack_require__(2867);
       const VALID_LOADING_VALUES =
         /* unused pure expression or super */ null && [
           "lazy",
@@ -830,7 +830,7 @@
       /***/
     },
 
-    /***/ 9254: /***/ (__unused_webpack_module, exports) => {
+    /***/ 9866: /***/ (__unused_webpack_module, exports) => {
       "use strict";
       /**
        * A shared function, used on both client and server, to generate a SVG blur placeholder.
@@ -885,7 +885,7 @@
       /***/
     },
 
-    /***/ 9331: /***/ (
+    /***/ 3484: /***/ (
       __unused_webpack_module,
       exports,
       __webpack_require__
@@ -912,10 +912,10 @@
         },
       });
       const _interop_require_default = __webpack_require__(4345);
-      const _getimgprops = __webpack_require__(2097);
-      const _imagecomponent = __webpack_require__(1112);
+      const _getimgprops = __webpack_require__(1607);
+      const _imagecomponent = __webpack_require__(7481);
       const _imageloader = /*#__PURE__*/ _interop_require_default._(
-        __webpack_require__(6067)
+        __webpack_require__(3463)
       );
       function getImageProps(imgProps) {
         const { props } = (0, _getimgprops.getImgProps)(imgProps, {
@@ -947,7 +947,7 @@
       /***/
     },
 
-    /***/ 6067: /***/ (__unused_webpack_module, exports) => {
+    /***/ 3463: /***/ (__unused_webpack_module, exports) => {
       "use strict";
 
       Object.defineProperty(exports, "__esModule", {
@@ -982,7 +982,7 @@
       /***/
     },
 
-    /***/ 4765: /***/ (
+    /***/ 5780: /***/ (
       __unused_webpack_module,
       __webpack_exports__,
       __webpack_require__
@@ -999,8 +999,8 @@
 
       // EXTERNAL MODULE: ./node_modules/.pnpm/[email protected]/node_modules/react/jsx-runtime.js
       var jsx_runtime = __webpack_require__(3262);
-      // EXTERNAL MODULE: ./node_modules/.pnpm/next@file+..+main-repo+packages+next+next-packed.tgz_react-dom@19.0.0-rc-06d0b89e-20240801_re_432dyi3er6pnalgpduyz2i6axm/node_modules/next/image.js
-      var next_image = __webpack_require__(2692);
+      // EXTERNAL MODULE: ./node_modules/.pnpm/next@file+..+diff-repo+packages+next+next-packed.tgz_react-dom@19.0.0-rc-06d0b89e-20240801_re_wusbvjw3sleq6d7g7gklhdw6bm/node_modules/next/image.js
+      var next_image = __webpack_require__(8565);
       var image_default = /*#__PURE__*/ __webpack_require__.n(next_image); // CONCATENATED MODULE: ./pages/nextjs.png
       /* harmony default export */ const nextjs = {
         src: "/_next/static/media/nextjs.cae0b805.png",
@@ -1030,12 +1030,12 @@
       /***/
     },
 
-    /***/ 2692: /***/ (
+    /***/ 8565: /***/ (
       module,
       __unused_webpack_exports,
       __webpack_require__
     ) => {
-      module.exports = __webpack_require__(9331);
+      module.exports = __webpack_require__(3484);
 
       /***/
     },
@@ -1045,7 +1045,7 @@
     /******/ var __webpack_exec__ = (moduleId) =>
       __webpack_require__((__webpack_require__.s = moduleId));
     /******/ __webpack_require__.O(0, [2888, 9774, 179], () =>
-      __webpack_exec__(8316)
+      __webpack_exec__(8834)
     );
     /******/ var __webpack_exports__ = __webpack_require__.O();
     /******/ _N_E = __webpack_exports__;

Diff for 124-HASH.js

Diff too large to display

Diff for main-HASH.js

Diff too large to display

Diff for app-page-exp..ntime.dev.js

failed to diff

Diff for app-page.runtime.dev.js

Diff too large to display

Commit: 240f468

eps1lon

No tests makes this hard to review. The PR description indicates a fix but it's unclear what feature was fixed.

eps1lon · 2024-08-05T08:40:10Z

packages/next/src/shared/lib/normalized-asset-prefix.ts

-  // assetPrefix as a url
-  if (escapedAssetPrefix && escapedAssetPrefix.startsWith('://')) {
-    return escapedAssetPrefix.split('://', 2)[1]
+  // if assetPrefix is a URL, we return the pathname


This comment is just repeating what the code says. The "what" should only be explained if the code is not trivial which it should be 99% of the time. The "why" is more important and that's missing here.

Will keep this in my heart, thank you.

packages/next/src/shared/lib/normalized-asset-prefix.ts

packages/next/src/shared/lib/normalized-asset-prefix.test.ts

packages/next/src/client/components/react-dev-overlay/internal/helpers/get-socket-url.ts

devjiwonchoi · 2024-08-05T11:51:00Z

packages/next/src/client/components/react-dev-overlay/internal/helpers/get-socket-url.ts


-  return `${protocol}://${hostname}:${port}${prefix}`
+  const { hostname, port } = window.location
+  return `${protocol}//${hostname}${port ? `:${port}` : ''}${prefix}`


: shouldn't be added when port is undefined.

devjiwonchoi · 2024-08-05T11:51:15Z

packages/next/src/client/components/react-dev-overlay/internal/helpers/get-socket-url.ts

  } catch {}

-  return protocol === 'http:' ? 'ws' : 'wss'
+  return protocol === 'http:' ? 'ws:' : 'wss:'


added : for consistency

devjiwonchoi · 2024-08-05T11:53:24Z

packages/next/src/client/components/react-dev-overlay/internal/helpers/get-socket-url.ts

+  if (URL.canParse(prefix)) {
+    // since normalized asset prefix is ensured to be a URL format,
+    // we can safely replace the protocol
+    return prefix.replace(/^http/, 'ws')


We could be extra safe and use the URL object again, not just replacing the string http.

Added a unit test for `normalized-asset-prefix` update test Update packages/next/src/client/components/react-dev-overlay/internal/helpers/get-socket-url.ts Update packages/next/src/shared/lib/normalized-asset-prefix.test.ts

devjiwonchoi · 2024-08-06T08:51:26Z

packages/next/src/shared/lib/normalized-asset-prefix.test.ts

+    expect(normalizedAssetPrefix('https://example.com/path/to/asset')).toBe(
+      'example.com/path/to/asset'
+    )


@eps1lon At first, it was expected to remove the protocol because #67983 added this function to handle at getSocketUrl, and it simply added the ws protocol to the output.

next.js/packages/next/src/client/components/react-dev-overlay/internal/helpers/get-socket-url.ts

Lines 19 to 23 in cc7b9e6

// if original assetPrefix is a full URL with protocol

// we just update to use the correct `ws` protocol

if (assetPrefix?.replace(/^\/+/, '').includes('://')) {

return `${protocol}://${prefix}`

}

devjiwonchoi · 2024-08-06T08:53:01Z

packages/next/src/shared/lib/normalized-asset-prefix.test.ts

+  it('should return the URL when assetPrefix is a URL', () => {
+    expect(normalizedAssetPrefix('https://example.com/path/to/asset')).toBe(
+      'https://example.com/path/to/asset'
+    )


Now we return the parsed URL and let getSocketUrl replace the protocol.

refactor: use URL obj refactor: use URL canParse Co-authored-by: Sebastian Silbermann <[email protected]> refactor

ijjk added created-by: Next.js team PRs by the Next.js team. type: next labels Aug 5, 2024

devjiwonchoi mentioned this pull request Aug 5, 2024

test: add test for normalized-asset-prefix #68519

Merged

devjiwonchoi mentioned this pull request Aug 5, 2024

refactor: throw if asset-prefix is url in dev #68520

Closed

devjiwonchoi marked this pull request as ready for review August 5, 2024 07:22

devjiwonchoi requested review from eps1lon and ijjk August 5, 2024 07:30

devjiwonchoi force-pushed the 08-05-fix_normalize_asset_prefix_for_socket_if_start_with_http branch from c6fa55c to 409d532 Compare August 5, 2024 07:57

eps1lon reviewed Aug 5, 2024

View reviewed changes

devjiwonchoi commented Aug 5, 2024

View reviewed changes

devjiwonchoi changed the title ~~fix: normalize asset prefix for socket if start with http~~ fix: normalize-asset-prefix not properly handling URL assetPrefix Aug 5, 2024

devjiwonchoi changed the title ~~fix: normalize-asset-prefix not properly handling URL assetPrefix~~ fix: normalize-asset-prefix adding leading slash when URL assetPrefix is provided Aug 5, 2024

devjiwonchoi requested a review from eps1lon August 5, 2024 13:09

ijjk approved these changes Aug 5, 2024

View reviewed changes

devjiwonchoi force-pushed the 08-05-fix_normalize_asset_prefix_for_socket_if_start_with_http branch 2 times, most recently from 59e4a9a to 44f23ca Compare August 6, 2024 08:26

test: add test for normalized-asset-prefix (#68519)

dc1088a

Added a unit test for `normalized-asset-prefix` update test Update packages/next/src/client/components/react-dev-overlay/internal/helpers/get-socket-url.ts Update packages/next/src/shared/lib/normalized-asset-prefix.test.ts

devjiwonchoi force-pushed the 08-05-fix_normalize_asset_prefix_for_socket_if_start_with_http branch from 44f23ca to c721b8c Compare August 6, 2024 08:43

devjiwonchoi commented Aug 6, 2024

View reviewed changes

devjiwonchoi marked this pull request as draft August 6, 2024 09:55

devjiwonchoi added 2 commits August 6, 2024 19:24

fix: normalize asset prefix for socket if start with http

38d17da

refactor: use URL obj refactor: use URL canParse Co-authored-by: Sebastian Silbermann <[email protected]> refactor

test: normalize-asset-prefix should return url if is url

240f468

devjiwonchoi force-pushed the 08-05-fix_normalize_asset_prefix_for_socket_if_start_with_http branch from 7b64dc7 to 240f468 Compare August 6, 2024 10:28

devjiwonchoi marked this pull request as ready for review August 6, 2024 10:29

eps1lon approved these changes Aug 6, 2024

View reviewed changes

devjiwonchoi merged commit a5e2a02 into canary Aug 7, 2024

devjiwonchoi deleted the 08-05-fix_normalize_asset_prefix_for_socket_if_start_with_http branch August 7, 2024 06:49

devjiwonchoi mentioned this pull request Aug 19, 2024

Hot reload not working at latest version of Next.js #51162

Closed

1 task

github-actions bot added the locked label Aug 21, 2024

github-actions bot locked as resolved and limited conversation to collaborators Aug 21, 2024

	// assetPrefix as a url
	if (escapedAssetPrefix && escapedAssetPrefix.startsWith('://')) {
	return escapedAssetPrefix.split('://', 2)[1]
	}

	// if original assetPrefix is a full URL with protocol
	// we just update to use the correct `ws` protocol
	if (assetPrefix?.replace(/^\/+/, '').includes('://')) {
	return `${protocol}://${prefix}`
	}

fix: normalize-asset-prefix adding leading slash when URL assetPrefix is provided #68518

fix: normalize-asset-prefix adding leading slash when URL assetPrefix is provided #68518

Uh oh!

Conversation

devjiwonchoi commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why?

How?

Uh oh!

devjiwonchoi commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijjk commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tests Passed

Uh oh!

ijjk commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Stats from current PR

Uh oh!

eps1lon left a comment

Choose a reason for hiding this comment

Uh oh!

eps1lon Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

devjiwonchoi Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devjiwonchoi Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

devjiwonchoi Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

devjiwonchoi Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

devjiwonchoi Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

devjiwonchoi Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix: `normalize-asset-prefix` adding leading slash when URL `assetPrefix` is provided #68518

fix: `normalize-asset-prefix` adding leading slash when URL `assetPrefix` is provided #68518

devjiwonchoi commented Aug 5, 2024 •

edited

Loading

devjiwonchoi commented Aug 5, 2024 •

edited

Loading

ijjk commented Aug 5, 2024 •

edited

Loading

ijjk commented Aug 5, 2024 •

edited

Loading