Skip to content

Conversation

onsclom
Copy link
Contributor

@onsclom onsclom commented May 1, 2024

System environment variables would get set with empty strings in development which breaks some builds. This fixes that by using the v2 of /env/pull introduced in https://github.com/vercel/api/pull/27777.

onsclom added 3 commits May 1, 2024 11:44
This stops exposing system env vars in development.
The old test fails because we don't expose system env vars in development anymore.
Copy link

changeset-bot bot commented May 1, 2024

🦋 Changeset detected

Latest commit: 8f2c032

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
vercel Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

EndangeredMassa
EndangeredMassa previously approved these changes May 1, 2024
@@ -390,7 +390,7 @@ function exposeSystemEnvs(
) {
const envs: Env = {};

if (autoExposeSystemEnvs) {
if (autoExposeSystemEnvs && target !== 'development') {
envs['VERCEL'] = '1';
envs['VERCEL_ENV'] = target || 'development';
Copy link
Contributor

Choose a reason for hiding this comment

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

question (soft-blocking): If target is not development, do we still want to default VERCEL_ENV to "development" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is a mock, to match the api behavior it should not provide a default VERCEL_ENV for dev. I made that decision in https://github.com/vercel/api/pull/27777 because it feels more consistent to just not send any system env vars in dev. But, I'm open to changing the api behavior to make VERCEL_ENV and maybe VERCEL exceptions.

const exitCodePromise = env(client);
await expect(client.stderr).toOutput(
'Downloading `development` Environment Variables for Project vercel-env-pull'
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (soft-blocking): Add assertion (or new test) somewhere that development env pulls do not include system env vars at all.

@onsclom onsclom force-pushed the austinmerrick/do-not-pull-system-env-vars-in-dev branch from 7486c4b to 790d1aa Compare May 2, 2024 18:28
@onsclom onsclom added pr: automerge Automatically merge the PR when checks pass and removed pr: automerge Automatically merge the PR when checks pass labels May 2, 2024
@onsclom onsclom requested a review from EndangeredMassa May 7, 2024 16:22
@onsclom onsclom added the pr: automerge Automatically merge the PR when checks pass label May 14, 2024
@kodiakhq kodiakhq bot merged commit 446ac49 into main May 15, 2024
@kodiakhq kodiakhq bot deleted the austinmerrick/do-not-pull-system-env-vars-in-dev branch May 15, 2024 12:17
EndangeredMassa pushed a commit that referenced this pull request May 15, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @vercel/build-utils@8.2.0

### Minor Changes

- fix corepack detection for package manager version determination
([#11596](#11596))

### Patch Changes

- Fix triggering of ignored project settings node version warning
([#11550](#11550))

## vercel@34.2.0

### Minor Changes

- Stop sending system environment variables in dev
([#11526](#11526))

### Patch Changes

- Updated dependencies
\[[`d3c1267e2`](d3c1267),
[`ccd7eb1fb`](ccd7eb1)]:
    -   @vercel/build-utils@8.2.0
    -   @vercel/node@3.1.5
    -   @vercel/static-build@2.5.9

## @vercel/client@13.2.7

### Patch Changes

- Updated dependencies
\[[`d3c1267e2`](d3c1267),
[`ccd7eb1fb`](ccd7eb1)]:
    -   @vercel/build-utils@8.2.0

## @vercel/gatsby-plugin-vercel-builder@2.0.31

### Patch Changes

- Updated dependencies
\[[`d3c1267e2`](d3c1267),
[`ccd7eb1fb`](ccd7eb1)]:
    -   @vercel/build-utils@8.2.0

## @vercel/node@3.1.5

### Patch Changes

- Updated dependencies
\[[`d3c1267e2`](d3c1267),
[`ccd7eb1fb`](ccd7eb1)]:
    -   @vercel/build-utils@8.2.0

## @vercel/static-build@2.5.9

### Patch Changes

-   Updated dependencies \[]:
    -   @vercel/gatsby-plugin-vercel-builder@2.0.31

## @vercel-internals/types@1.0.36

### Patch Changes

- Updated dependencies
\[[`d3c1267e2`](d3c1267),
[`ccd7eb1fb`](ccd7eb1)]:
    -   @vercel/build-utils@8.2.0

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
trek added a commit that referenced this pull request Oct 24, 2024
…12358)

In #11526 we stopped pulling down
environment variables that are set by the runtime into `.env` files
since this really is an appropriate location for them and blank values
can cause issues for some customers.

However, we were relying on these so that `vercel dev` had a similar set
of variables as the platform at runtime. Losing these was a breaking
change.

This PR sets them within the development sever itself to better simulate
our actual runtime environment. I wasn't quite sure of the use of
`projectSettings?.autoExposeSystemEnvs`
[here](https://github.com/vercel/vercel/blob/804b94fa6177198728b1d6cd497aad787b452130/packages/cli/src/util/dev/server.ts#L709-L711)
and potentially the correct location for these is within that `if`
block?

Closes #2878

---------

Co-authored-by: Nathan Rajlich <n@n8.io>
QuietCraftsmanship pushed a commit to QuietCraftsmanship/Vercel that referenced this pull request Jul 6, 2025
…12358)

In vercel/vercel#11526 we stopped pulling down
environment variables that are set by the runtime into `.env` files
since this really is an appropriate location for them and blank values
can cause issues for some customers.

However, we were relying on these so that `vercel dev` had a similar set
of variables as the platform at runtime. Losing these was a breaking
change.

This PR sets them within the development sever itself to better simulate
our actual runtime environment. I wasn't quite sure of the use of
`projectSettings?.autoExposeSystemEnvs`
[here](https://github.com/vercel/vercel/blob/9537e8c1eb2428e76c8ccf3a52c2e7e8de42e96a/packages/cli/src/util/dev/server.ts#L709-L711)
and potentially the correct location for these is within that `if`
block?

Closes #2878

---------

Co-authored-by: Nathan Rajlich <n@n8.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: automerge Automatically merge the PR when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants