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

fix: setting assetPrefix to URL format breaks HMR #70040

Merged
merged 3 commits into from
Sep 12, 2024
Merged

fix: setting assetPrefix to URL format breaks HMR #70040

merged 3 commits into from
Sep 12, 2024

Conversation

### Why?

In #68622 we fixed a bug that HMR
breaks when `assetPrefix` is set to full URL.
The fix was targeted to the edge case where the user may set `localhost`
to the `assetPrefix`, and technically, when the request is valid the HMR
should work.

However, we do not recommend this pattern as the `assetPrefix` is
intended to be used for setting a CDN URL.

### How?

Therefore we modify the docs to gently imply that the `assetPrefix` is
for CDN URL.
Also, removed the warning log that could be confusing whether we support
it or not.
Setting localhost will still work, but we do not mention about it within
the app.
…fix` is provided (#68518)

### Why?

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


https://github.com/vercel/next.js/blob/37df9ab243690187725a236cbf2debbcaeb33cfa/packages/next/src/shared/lib/normalized-asset-prefix.ts#L4-L7

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).
@ijjk ijjk added created-by: Next.js team PRs by the Next.js team. Documentation Related to Next.js' official documentation. tests type: next labels Sep 12, 2024
@ijjk
Copy link
Member

ijjk commented Sep 12, 2024

Stats from current PR

Default Build
General Overall increase ⚠️
vercel/next.js 14-2-1 vercel/next.js asset-prefix Change
buildDuration 16.3s 14.9s N/A
buildDurationCached 8.1s 6.6s N/A
nodeModulesSize 200 MB 200 MB ⚠️ +2.97 kB
nextStartRea..uration (ms) 404ms 416ms N/A
Client Bundles (main, webpack)
vercel/next.js 14-2-1 vercel/next.js asset-prefix Change
1103-HASH.js gzip 31.6 kB 31.6 kB N/A
1a9f679d-HASH.js gzip 53.7 kB 53.7 kB N/A
335-HASH.js gzip 5.05 kB 5.05 kB
7953.HASH.js gzip 181 B 181 B
framework-HASH.js gzip 44.9 kB 44.9 kB
main-app-HASH.js gzip 243 B 244 B N/A
main-HASH.js gzip 32.2 kB 32.3 kB N/A
webpack-HASH.js gzip 1.68 kB 1.68 kB N/A
Overall change 50.1 kB 50.1 kB
Legacy Client Bundles (polyfills)
vercel/next.js 14-2-1 vercel/next.js asset-prefix Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js 14-2-1 vercel/next.js asset-prefix Change
_app-HASH.js gzip 195 B 197 B N/A
_error-HASH.js gzip 184 B 184 B
amp-HASH.js gzip 501 B 506 B N/A
css-HASH.js gzip 323 B 324 B N/A
dynamic-HASH.js gzip 1.82 kB 1.82 kB N/A
edge-ssr-HASH.js gzip 259 B 258 B N/A
head-HASH.js gzip 350 B 352 B N/A
hooks-HASH.js gzip 372 B 371 B N/A
image-HASH.js gzip 4.23 kB 4.23 kB
index-HASH.js gzip 259 B 258 B N/A
link-HASH.js gzip 2.68 kB 2.68 kB N/A
routerDirect..HASH.js gzip 316 B 315 B N/A
script-HASH.js gzip 387 B 387 B
withRouter-HASH.js gzip 311 B 310 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 4.9 kB 4.9 kB
Client Build Manifests
vercel/next.js 14-2-1 vercel/next.js asset-prefix Change
_buildManifest.js gzip 483 B 483 B
Overall change 483 B 483 B
Rendered Page Sizes
vercel/next.js 14-2-1 vercel/next.js asset-prefix Change
index.html gzip 528 B 527 B N/A
link.html gzip 542 B 540 B N/A
withRouter.html gzip 524 B 522 B N/A
Overall change 0 B 0 B
Edge SSR bundle Size
vercel/next.js 14-2-1 vercel/next.js asset-prefix Change
edge-ssr.js gzip 95.4 kB 95.4 kB
page.js gzip 3.07 kB 3.06 kB N/A
Overall change 95.4 kB 95.4 kB
Middleware size
vercel/next.js 14-2-1 vercel/next.js asset-prefix Change
middleware-b..fest.js gzip 661 B 657 B N/A
middleware-r..fest.js gzip 156 B 156 B
middleware.js gzip 25.5 kB 25.5 kB N/A
edge-runtime..pack.js gzip 839 B 839 B
Overall change 995 B 995 B
Next Runtimes
vercel/next.js 14-2-1 vercel/next.js asset-prefix Change
app-page-exp...dev.js gzip 171 kB 171 kB
app-page-exp..prod.js gzip 98.1 kB 98.1 kB
app-page-tur..prod.js gzip 99.8 kB 99.8 kB
app-page-tur..prod.js gzip 94.1 kB 94.1 kB
app-page.run...dev.js gzip 145 kB 145 kB
app-page.run..prod.js gzip 92.6 kB 92.6 kB
app-route-ex...dev.js gzip 22 kB 22 kB
app-route-ex..prod.js gzip 15.5 kB 15.5 kB
app-route-tu..prod.js gzip 15.5 kB 15.5 kB
app-route-tu..prod.js gzip 15.2 kB 15.2 kB
app-route.ru...dev.js gzip 21.7 kB 21.7 kB
app-route.ru..prod.js gzip 15.2 kB 15.2 kB
pages-api-tu..prod.js gzip 9.58 kB 9.58 kB
pages-api.ru...dev.js gzip 9.85 kB 9.85 kB
pages-api.ru..prod.js gzip 9.57 kB 9.57 kB
pages-turbo...prod.js gzip 22.5 kB 22.5 kB
pages.runtim...dev.js gzip 23.2 kB 23.2 kB
pages.runtim..prod.js gzip 22.5 kB 22.5 kB
server.runti..prod.js gzip 51.5 kB 51.5 kB
Overall change 955 kB 955 kB
build cache
vercel/next.js 14-2-1 vercel/next.js asset-prefix Change
0.pack gzip 1.61 MB 1.61 MB N/A
index.pack gzip 113 kB 112 kB N/A
Overall change 0 B 0 B
Diff details
Diff for edge-ssr.js

Diff too large to display

Diff for main-HASH.js

Diff too large to display

Commit: 725c6da

@ijjk ijjk marked this pull request as ready for review September 12, 2024 18:22
@ijjk ijjk merged commit 276ddf3 into 14-2-1 Sep 12, 2024
52 of 57 checks passed
@ijjk ijjk deleted the asset-prefix branch September 12, 2024 18:24
@stephenhmarsh
Copy link

Fwiw, this work introduced a bug for us similar to #65412

Downgrading to 14.2.10 was the only thing we tried that worked.

@github-actions github-actions bot added the locked label Oct 2, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
created-by: Next.js team PRs by the Next.js team. Documentation Related to Next.js' official documentation. locked tests type: next
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants