Skip to content

Commit

Permalink
Do not re-assign process.env (#46914)
Browse files Browse the repository at this point in the history
## Checklist

- [ ] Related issues linked using `fixes #number` 
  - no related issue exists, happy to open one if desired
- [x] Tests added
- not sure if specific tests are needed? there is an integration test
for environment variables, and Next.js relies a lot on passing
information through environment variables; i'd expect everything to
break if this change broke environment variable handling
- [x] Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md
  - no new errors, does not apply

### What?

Re-assigning `process.env` substitutes the "magic object" that sets
environment variables at the process level with an ordinary JavaScript
object. This causes environment variables that are set after
`process.env` is re-assigned to not be visible to native add-ons.

See [this Node.js issue][issue] and [this reproduction case][repro] for
details.

[issue]: nodejs/node#46996
[repro]: https://github.com/unflxw/nodejs-process-env-addons-repro

### Why?

In general, paraphrasing the maintainer in the Node.js issue,
re-assigning `process.env` is not a thing you should do. More
specifically, I'm trying to use Next.js' experimental OpenTelemetry
support with AppSignal's Node.js integration, which also uses
OpenTelemetry.

The AppSignal Node.js package sets environment variables in order to
configure a long-running process, which is then launched through a
native add-on. Because of the re-assignment of `process.env` that occurs
early in Next.js' lifecycle process, by the time the AppSignal Node.js
package sets environment variables, it's setting them in an ordinary
JavaScript object that Next.js left in the global `process` object, not
in the magic one created by the Node.js runtime.

This means that these environment variables are not _actually_ being set
for the process at the OS level, and therefore they're also not set for
the native add-on, or for the long-running process it spawns.

### How?

A `replaceProcessEnv` function is implemented that takes an environment
object as an argument, and applies the difference between that
environment object and the current environment to the existing
`process.env` object. This function is used instead of re-assigning
`process.env`.

Co-authored-by: JJ Kasper <jj@jjsweb.site>
  • Loading branch information
unflxw and ijjk authored Mar 9, 2023
1 parent 9f08ef8 commit 71dbcac
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 1 deletion.
14 changes: 13 additions & 1 deletion packages/next-env/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,18 @@ type Log = {
error: (...args: any[]) => void
}

function replaceProcessEnv(sourceEnv: Env) {
Object.keys(process.env).forEach((key) => {
if (sourceEnv[key] === undefined || sourceEnv[key] === '') {
delete process.env[key]
}
})

Object.entries(sourceEnv).forEach(([key, value]) => {
process.env[key] = value
})
}

export function processEnv(
loadedEnvFiles: LoadedEnvFiles,
dir?: string,
Expand Down Expand Up @@ -94,7 +106,7 @@ export function loadEnvConfig(
if (combinedEnv && !forceReload) {
return { combinedEnv, loadedEnvFiles: cachedLoadedEnvFiles }
}
process.env = Object.assign({}, initialEnv)
replaceProcessEnv(initialEnv)
previousLoadedEnvFiles = cachedLoadedEnvFiles
cachedLoadedEnvFiles = []

Expand Down
9 changes: 9 additions & 0 deletions test/unit/preserve-process-env.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { loadEnvConfig } from '../../packages/next-env/'

describe('preserve process env', () => {
it('should not reassign `process.env`', () => {
const originalProcessEnv = process.env
loadEnvConfig('.')
expect(Object.is(originalProcessEnv, process.env)).toBeTrue()
})
})

0 comments on commit 71dbcac

Please sign in to comment.