fix: `normalize-asset-prefix` adding leading slash when URL `assetPrefix` is provided by devjiwonchoi · Pull Request #68518 · vercel/next.js

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/react@19.0.0-rc-06d0b89e-20240801/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

-  // 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.

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


-  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

  } catch {}

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


added : for consistency

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

+  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

+    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

+  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 <silbermann.sebastian@gmail.com> 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 <silbermann.sebastian@gmail.com> 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 merged 3 commits into
canaryfrom
08-05-fix_normalize_asset_prefix_for_socket_if_start_with_http

devjiwonchoi commented Aug 5, 2024 •

edited

Loading

Uh oh!

devjiwonchoi commented Aug 5, 2024 •

edited

Loading

Uh oh!

ijjk commented Aug 5, 2024 •

edited

Loading

Uh oh!

ijjk commented Aug 5, 2024 •

edited

Loading

Uh oh!

eps1lon left a comment

Uh oh!

eps1lon Aug 5, 2024

Uh oh!

devjiwonchoi Aug 5, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devjiwonchoi Aug 5, 2024

Uh oh!

devjiwonchoi Aug 5, 2024

Uh oh!

devjiwonchoi Aug 5, 2024

Uh oh!

devjiwonchoi Aug 6, 2024

Uh oh!

devjiwonchoi Aug 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// 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}`
	}

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!

3 participants

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