Skip to content

Commit

Permalink
Recommend using next CLI for Docker entrypoint and not yarn (#29024)
Browse files Browse the repository at this point in the history 
Fixes #29023 

## Bug

- [x] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [x] Errors have helpful link attached, see `contributing.md`

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation / Examples

- [x] Make sure the linting passes
  • Loading branch information
@stefee
stefee committed Nov 1, 2021
1 parent a89e651 commit 5abfea3
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 2 deletions.
4 changes: 3 additions & 1 deletion docs/deployment.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,12 +127,14 @@ USER nextjs

EXPOSE 3000

ENV PORT 3000

# Next.js collects completely anonymous telemetry data about general usage.
# Learn more here: https://nextjs.org/telemetry
# Uncomment the following line in case you want to disable telemetry.
# ENV NEXT_TELEMETRY_DISABLED 1

CMD ["yarn", "start"]
CMD ["node_modules/.bin/next", "start"]

This comment has been minimized.

Sign in to view

Copy link
@Duncan-Brain

Duncan-Brain Nov 3, 2021

Hello, does the return to using the next entrypoint make the package.json copy (line 124) unnecessary? Looks like they were they were added at the same time in this merge: https://github.com/vercel/next.js/pull/23486/files/e03cbf775ae252b2ade14c4171d5bde8e7bfd725#r602910215

This comment has been minimized.

Sign in to view

Copy link
@stefee

stefee Nov 5, 2021 •
edited

Author Contributor

Thanks for this question, I didn't think about the significance of the start script when I made this change. My focus was removing the yarn "proxy" process as the Docker entrypoint and using next process directly instead, for the reasons I outlined in the issue.

https://github.com/vercel/next.js/blob/e2506411a1cf287326082d2c326e58e8cc27a7a9/examples/with-docker/README.md#deploying-to-google-cloud-run

The start script in package.json has been modified to accept a PORT environment variable (for compatibility with Google Cloud Run).

I am actually not familiar with Google Cloud Run and what the significance of the "start" npm script is in that context. Maybe @leerob knows more and is able to shed some light on this.

It might be the case that the npm script itself has no particular significance, in which case we can re-word this line in the readme to reflect that (and perhaps remove the script entirely, although it might still be useful for local development). If the script does has some significance then we might adjust implementation based on that - i.e. it might make sense to have a separate examples/ directory for Google Cloud Run in particular.

This comment has been minimized.

Sign in to view

Copy link
@stefee

stefee Nov 5, 2021 •
edited

Author Contributor

I am re-reading the comments on the linked PR and wondering if perhaps we just need to add -p $PORT to the CMD line here in order to keep support for GCR, and we can remove the "start" script? I think it's not strictly needed.

Edit: looks like next will pick up $PORT without needing -p flag https://nextjs.org/docs/api-reference/cli#production

This comment has been minimized.

Sign in to view

Copy link
@Duncan-Brain

Duncan-Brain Nov 5, 2021 •
edited

I am a little out of my depth here but I read the comment as style/process choice to make package.json the source of truth for the start script while also solving that port issue.

In your issue you also suggested using tini which also seems to follow node best practices . So it could be a way to keep the style of using package.json if that is important to your team.

This comment has been minimized.

Sign in to view

Copy link
@stefee

stefee Nov 5, 2021 •
edited

Author Contributor

I am a little out of my depth here but I read the comment as style/process choice to make package.json the source of truth for the start script while also solving that port issue.

Yes, that is also my reading

In your issue you also suggested using tini which also seems to follow node best practices . So it could be a way to keep the style of using package.json if that is important to your team.

Just to note: AFAIK using tini doesn't actually solve the problems associated with using npm script as Docker entrypoint - it does help with a different problem that Node.js is not designed to be used as PID 1 on Linux, but that is an unrelated issue. The issue with using npm script as Docker entrypoint is there is no guarantee that process signals will be forwarded to the child process - tini doesn't help you with that problem, but it will help you by providing a process wrapper which means that for example, Node.js will not be PID 1. They are two different problems.

This comment has been minimized.

Sign in to view

Copy link
@stefee

stefee Nov 5, 2021

Author Contributor

I've opened #31053 just in case it is helpful, but I am not 100% confident it is correct
```

Make sure to place this Dockerfile in the root folder of your project.
Expand Down
4 changes: 3 additions & 1 deletion examples/with-docker/Dockerfile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,11 @@ USER nextjs

EXPOSE 3000

ENV PORT 3000

# Next.js collects completely anonymous telemetry data about general usage.
# Learn more here: https://nextjs.org/telemetry
# Uncomment the following line in case you want to disable telemetry.
# ENV NEXT_TELEMETRY_DISABLED 1

CMD ["yarn", "start"]
CMD ["node_modules/.bin/next", "start"]

0 comments on commit 5abfea3

Please sign in to comment.