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

[isHMRRequest] assetPrefix can never be part of req.url, so remove it #66126

Closed

Conversation

edwinveldhuizen
Copy link

@edwinveldhuizen edwinveldhuizen commented May 23, 2024

What?

Remove assetPrefix from the isHMRRequest check which was accidentally added in #59471

Why?

assetPrefix is supposed to be a full domain (https://nextjs.org/docs/pages/api-reference/next-config-js/assetPrefix)
Therefore it can never be part of req.url

Currently this is breaking the hot module reload for dev environments that rely on the assetPrefix to be set

Fixes #51162

@ijjk
Copy link
Member

ijjk commented May 23, 2024

Allow CI Workflow Run

  • approve CI run for commit: a59112b

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

1 similar comment
@ijjk
Copy link
Member

ijjk commented May 23, 2024

Allow CI Workflow Run

  • approve CI run for commit: a59112b

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@Netail
Copy link
Contributor

Netail commented May 23, 2024

assetPrefix is supposed to be a full domain

assetPrefix should also be able to work with a relative path afaik, as there are some checks somewhere to see if it's relative or not. Maybe unclear from the docs

@edwinveldhuizen
Copy link
Author

edwinveldhuizen commented May 23, 2024

assetPrefix is supposed to be a full domain

assetPrefix should also be able to work with a relative path afaik, as there are some checks somewhere to see if it's relative or not. Maybe unclear from the docs

The docs state:

Good to know: Next.js 9.5+ added support for a customizable Base Path, which is better suited for hosting your application on a sub-path like /docs. We do not suggest you use a custom Asset Prefix for this use case.

@Netail
Copy link
Contributor

Netail commented May 23, 2024

assetPrefix is supposed to be a full domain

assetPrefix should also be able to work with a relative path afaik, as there are some checks somewhere to see if it's relative or not. Maybe unclear from the docs

The docs state:

Good to know: Next.js 9.5+ added support for a customizable Base Path, which is better suited for hosting your application on a sub-path like /docs. We do not suggest you use a custom Asset Prefix for this use case.

Mhhmmm, with "this use case" I think they mean all your page routes not your bundles.

With basePath you are not able to prefix only your bundles, e.g. having the routes
/winkels
/winkel/1234
with the bundle serving on /stores-app/_next/...

@devjiwonchoi
Copy link
Member

Thank you for the PR!
We have a PR with a slight different fix, feel free to review!
x-ref: #68622

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 22, 2024
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.

Hot reload not working at latest version of Next.js
4 participants