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

[cli] Add experimental corepack support #7871

Merged
merged 26 commits into from
Jun 1, 2022
Merged

[cli] Add experimental corepack support #7871

merged 26 commits into from
Jun 1, 2022

Conversation

styfle
Copy link
Member

@styfle styfle commented May 25, 2022

This PR adds support for experimental corepack to vc build.

Since this is still experimental, we only enable if the env var ENABLE_EXPERIMENTAL_COREPACK=1 is set.

Tests

  • The code changed/added as part of this PR has been covered with tests
  • All tests pass locally with yarn test-unit

Copy link
Contributor

@Ethan-Arrowood Ethan-Arrowood left a comment

Choose a reason for hiding this comment

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

lgtm! just needs a test

packages/cli/src/commands/build.ts Outdated Show resolved Hide resolved
packages/cli/test/integration.js Show resolved Hide resolved
packages/cli/test/integration.js Show resolved Hide resolved
packages/cli/src/commands/build.ts Outdated Show resolved Hide resolved
packages/cli/src/commands/build.ts Outdated Show resolved Hide resolved
packages/cli/test/integration.js Show resolved Hide resolved
@EndangeredMassa
Copy link
Contributor

Looks good!

Copy link
Member

@TooTallNate TooTallNate left a comment

Choose a reason for hiding this comment

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

Suggested a couple minor fixes.

packages/cli/src/util/build/corepack.ts Outdated Show resolved Hide resolved
packages/cli/src/util/build/corepack.ts Outdated Show resolved Hide resolved
@styfle styfle requested a review from TooTallNate June 1, 2022 14:19
@kodiakhq kodiakhq bot merged commit 68cb23c into main Jun 1, 2022
@kodiakhq kodiakhq bot deleted the enable-corepack branch June 1, 2022 14:31
kodiakhq bot pushed a commit that referenced this pull request Jul 1, 2022
…repend (#8065)

This PR fixes a bug where corepack (#7871) was not correctly setup because the lockfile autodetection and node version autodetection was overriding the PATH.

It also fixes a bug where the log output was printed twice because we incorrectly prepended the PATH twice.
kodiakhq bot pushed a commit that referenced this pull request Jul 11, 2022
This debug log was originally added in #7871 because corepack has no output by default. In particular, it was nice to see the first deployment was not stalled when the package manager is being installed.

That being said, this gets noisy really fast because cache detections also print a log line.
For example, here's a deployment that prints 3 times:

```
Detected ENABLE_EXPERIMENTAL_COREPACK=1 and "[email protected]" in package.json
Running "install" command: `npm install --prefix=.. --no-audit --engine-strict=false`...
2022-07-11T18:27:00.696Z corepack Reusing [email protected]
356 packages are looking for funding
Running "npm run vercel-build"
2022-07-11T18:27:06.664Z corepack Reusing [email protected]
> [email protected] vercel-build
> npm run buildonly && npm run build:rss
2022-07-11T18:27:07.088Z corepack Reusing [email protected]
> [email protected] buildonly
> next build
```

I think its best to let users add this env var themselves if they want to debug what corepack is doing, so this PR removes that environment variable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants