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

feat(node): add docker as a build target #14475

Merged
merged 1 commit into from
Jan 20, 2023
Merged

Conversation

ndcunningham
Copy link
Contributor

@ndcunningham ndcunningham commented Jan 18, 2023

This adds a docker configuration to a current node project (standalone / integrated)

It should create a Dockerfile which builds an image based off the output path for your node project
Running the command

npx nx g @nrwl/node:app --framework=fastify api --docker

Should create a folder structure similar to

Screenshot 2023-01-19 at 2 55 28 PM

@vercel
Copy link

vercel bot commented Jan 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
nx-dev ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 20, 2023 at 10:06PM (UTC)

docs/generated/packages/node/generators/setup-docker.json Outdated Show resolved Hide resolved
@@ -0,0 +1,6 @@
FROM node:alpine
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want something more elaborate in the lines of

ROM docker.io/node:lts-alpine

ENV NODE_ENV production

WORKDIR <%= app %>
RUN npm install -g npm@latest

COPY <%= buildLocation %> ./
RUN npm install --omit=dev

RUN chown -R node:node .
USER node

CMD [ "node", "<%= main %>" ]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FROM docker.io/node:lts-alpine makse sense, I wouldn't do npm install in Dockerfile since the dist should already be present andd bundled.

If users have external dependencies (e.g. not bundled in) then it makes sense to include install step -- I think they can add the install it manually for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also for can run COPY --chown node:node <%= buildLocation %> .

Like:

RUN addgroup --system node && \
          adduser --system -G node node

COPY --chown node:node <%= buildLocation %> .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't do npm install in Dockerfile since the dist should already be present and bundled.

In my setup (node app / webpack) the deps are not bundled.
I was not aware of the possibility that deps could be bundled for a node app.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's possible if you use esbuild or webpack... not plain tsc though. I highly recommend it since you benefit from a smaller image size as well. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't though tsc does bundling or tree shaking!

docs/generated/packages/node/generators/setup-docker.json Outdated Show resolved Hide resolved
lerna.json Outdated Show resolved Hide resolved
packages/node/src/generators/setup-docker/setup-docker.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,6 @@
FROM node:alpine
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FROM docker.io/node:lts-alpine makse sense, I wouldn't do npm install in Dockerfile since the dist should already be present andd bundled.

If users have external dependencies (e.g. not bundled in) then it makes sense to include install step -- I think they can add the install it manually for now.

@@ -0,0 +1,6 @@
FROM node:alpine
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also for can run COPY --chown node:node <%= buildLocation %> .

Like:

RUN addgroup --system node && \
          adduser --system -G node node

COPY --chown node:node <%= buildLocation %> .

@ndcunningham ndcunningham self-assigned this Jan 19, 2023
@ndcunningham ndcunningham marked this pull request as ready for review January 19, 2023 22:00
@ndcunningham ndcunningham changed the title feat(node): WIP Add docker feat(node): add docker as a build target Jan 19, 2023
@github-actions
Copy link

github-actions bot commented Mar 8, 2023

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants