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.

Sign up for GitHub

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

Jump to bottom

docker.mdx: use --omit=dev for npm over deprecated --production #8226

Merged
merged 2 commits into from
May 14, 2024
Merged

docker.mdx: use --omit=dev for npm over deprecated --production #8226

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
51f1f58
docker.mdx: use `--omit=dev` for npm instead of deprecated `--product…
corneliusroemer May 8, 2024
27a9915
Merge branch 'main' into patch-1
sarah11918 May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/content/docs/en/recipes/docker.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,10 +148,10 @@ WORKDIR /app
COPY package.json package-lock.json ./
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Suggestion/Nitpick: While ./ works fine, perhaps /app/ would convey more information without relying on the previous state of the WORKDIR.

Copy link
Contributor

Choose a reason for hiding this comment

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

both are ok I think


FROM base AS prod-deps
RUN npm install --production
RUN npm install --omit=dev
sarah11918 marked this conversation as resolved.
Show resolved Hide resolved

FROM base AS build-deps
RUN npm install --production=false
RUN npm install
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Suggestion: Instead of install, it would be safer to use ci. At this stage, installing is essentially a clean install, but if someone copies this into a different context, they might face some unintended behaviors.

Suggested change
RUN npm install
RUN npm ci

Copy link
Member

Choose a reason for hiding this comment

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

I will keep this consistent with the original instruction, just removing the flag. If there are changes to be made to the recipe otherwise, we can always revisit!


FROM build-deps AS build
COPY . .
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Niptick: While it works, I would instead specify the intended folders to copy. This solution heavily relies on .dockerignore being correct and not allowing anything unwanted to be copied.

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Suggestion/Nitpick: While . works fine, perhaps /app/ would convey more information without relying on the previous state of the WORKDIR.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's different working styles, I guess the doc purpose is only to convey examples, it's hard to cover all use cases, and sometimes people use them in different context, some even develop with live reload under docker. relying on workdir also prevents changing it everywhere in case of rename.

Copy link
Sponsor Contributor

@adaliszk adaliszk May 9, 2024 •
edited

Choose a reason for hiding this comment

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

I would still advise against using COPY . . to avoid beginner mistakes. With HMR you would mount the files, and that is fine, but when you COPY, you do copy files into your image! You can very easily make a mistake and blow up your image size and even leak sensitive information with things like .env.

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

FYI: https://discord.com/channels/830184174198718474/1245118392793436201

Expand Down