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

fix: dockerfile npm run build #61466

Merged
merged 2 commits into from
Jan 31, 2024
Merged

fix: dockerfile npm run build #61466

merged 2 commits into from
Jan 31, 2024

Conversation

stefangeneralao
Copy link
Contributor

Removed comment suggesting to use npm as an alternative but it's already configured by default.

Removed comment suggesting to use npm as an alternative but it's already configured by default.
@stefangeneralao stefangeneralao requested review from a team as code owners January 31, 2024 18:34
@stefangeneralao stefangeneralao requested review from manovotny and lydiahallie and removed request for a team January 31, 2024 18:34
@ijjk ijjk added the examples Issue/PR related to examples label Jan 31, 2024
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

I'm guessing this should say pnpm run build or yarn run build but it can probably be removed

@styfle styfle added the CI approved Approve running CI for fork label Jan 31, 2024
@stefangeneralao
Copy link
Contributor Author

@styfle Yes, exactly right. The file had the following two days ago:

RUN yarn build

# If using npm comment out above and use below instead
# RUN npm run build

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

I just pushed an update to add the if/else, thanks!

@styfle styfle changed the title Remove legacy comment fix: dockerfile npm run build Jan 31, 2024
@styfle styfle enabled auto-merge (squash) January 31, 2024 18:45
@styfle styfle disabled auto-merge January 31, 2024 18:45
@styfle styfle enabled auto-merge (squash) January 31, 2024 18:45
@stefangeneralao
Copy link
Contributor Author

Beautiful! Thanks

@styfle styfle merged commit e0c63ac into vercel:canary Jan 31, 2024
30 checks passed
styfle pushed a commit that referenced this pull request Feb 1, 2024
### Adding or Updating Examples

- Example:
[with-docker](https://github.com/vercel/next.js/tree/canary/examples/with-docker)
- Update: The latest change introduced in #61466 on the building process
of NextJS when the package manager is PNPM is currently throwing an
error (it works for npm though).

**To replicate the error:**

1. Checkout on the **with-docker** example
2. Install with PNPM so that it generates the pnpm-lock.yaml
3. Build the Docker image -> This will fail on the _builder stage_ step
that runs `pnpm run build`.

**Possible Solution:**
- Run `corepack enable pnpm` before building with pnpm
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI approved Approve running CI for fork examples Issue/PR related to examples locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants