-
Notifications
You must be signed in to change notification settings - Fork 4
perf: Multistage Docker build #2200
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not tested locally since I don't have Docker installed. But the change looks good.
|
||
EXPOSE 3000 | ||
|
||
# Instead of running npm start; handle signals (SIGINT/SIGTERM) properly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newer versions of Next.js require ENV HOSTNAME=0.0.0.0
. Might be worth adding it today to not get any surprises when you upgrade Next.js in the future.
See official Docker example: https://github.com/vercel/next.js/blob/62ac6d040183d48a5d83d2b197826c0c471caa62/examples/with-docker/Dockerfile#L65
# The file that Next.js generates is CommonJS, but the frontend folder has a | ||
# package.json with type:module, so node expects ESM when files have a .js | ||
# extension. | ||
# | ||
# This should eventually be fixed in Next.js, but for the time being adjusting | ||
# the extension seems to be the easiest path forward (thanks @wereHamster!) | ||
RUN mv ./app/server.js ./app/server.cjs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This project does not use type:module, so renaming the file to add the .cjs extension is not necessary. And I believe the latest Next.js works without this workaround (if you ever decide to switch to type:module).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (2)
- .dockerignore: Language not supported
- Dockerfile: Language not supported
Closes #
This PR...
How to test