sindresorhus / execa

Process execution for humans
MIT License
6.79k stars 214 forks source link

Unable to extend env when `ProcessEnv` has been manipulated #1132

Closed ehyland closed 3 months ago

ehyland commented 3 months ago

When a non optional property has been defined on ProcessEnv by interface merging, it is not possible to use execa's env & extendEnv options to add additional environment variables to execa process.

Typescript playground example

Example

import { config } from "~/config.server";
import { execa } from "execa";

// from @remix-run/node
interface ProcessEnv {
  NODE_ENV: "development" | "production" | "test";
}

// attempting to extend process env results in TS error "No overload matches this call."
const $ = execa({
  stdio: "inherit",
  extendEnv: true,
  env: {
    DATABASE_URL: config.DATABASE_URL,
  },
});

await $`prisma db push --accept-data-loss`;

Suggestion

Instead of env option being typed as typeof import('node:process').env. Maybe it should be typed as Record<string, string | undefined>

ehmicky commented 3 months ago

Hi @ehyland,

This seems to be a bug with @remix-run/node, not Execa. Namely, the same behavior happens when using node:child_process instead of Execa (TypeScript playground).

import {spawn} from 'node:child_process';
import "@remix-run/node";

spawn('echo', ['example'], {
  env: {
    DATABASE_URL: 'DATABASE_URL'
  }
});
No overload matches this call.
  The last overload gave the following error.
    Property 'NODE_ENV' is missing in type '{ DATABASE_URL: string; }' but required in type 'ProcessEnv'.

The problem is: the interface merging done by @remix-run/node is making NODE_ENV a required property. Your example above is not defining env.NODE_ENV, therefore typing fails, but that's the expected behavior from the additional constraint Remix is adding.

I am not sure whether Remix did intend to force users into specifying env.NODE_ENV or whether they forgot to add ? in those specific types. From looking at their code (here and there, for instance), it looks like NODE_ENV is optional. However, the PR that added types for NODE_ENV incorrectly typed it as required (https://github.com/remix-run/remix/pull/983).

I would suggest one of the following:

import { execa, type Options } from 'execa';
import '@remix-run/node'

const $ = execa({
  stdio: "inherit",
  extendEnv: true,
  env: {
    DATABASE_URL: 'DATABASE_URL'
  } as unknown as Options['env'],
});
ehmicky commented 3 months ago

@ehyland Did this fix your issue?

ehyland commented 3 months ago

Hi @ehmicky, thanks for your response there

I got around this by setting NODE_ENV in options.env.

I can see this from both sides. While I would prefer they did not extend ProcessEnv, I don't think it's entirely incorrect.

In the remix app environment that they control NODE_ENV will always be set.

But it has an unintended consequence that if for some reason you want to define ProcessEnv, you now have to provide NODE_ENV.

Given the execa APIs env option is designed for extending the current process environment. Might it be more correct to type the option as Partial<ProcessEnv>?

ehmicky commented 3 months ago

Node.js uses ProcessEnv to define both process.env and the env option of child_process.spawn(). Even if process.env.NODE_PATH is always defined in Remix, typing ProcessEnv["NODE_PATH"] as required means that env.NODE_PATH must always be passed to child_process.spawn(), which was probably not intended by Remix.

As mentioned above, this problem is not specific to Execa: the same issue happens with child_process.spawn(). So that's definitely not something that should be fixed in Execa.

However, I would recommend opening an issue with Remix to make ProcessEnv["NODE_PATH"] optional.

ehyland commented 2 months ago

Yeah okay, makes sense to follow child_process module types. Thanks for the support @ehmicky

ehmicky commented 2 months ago

I created an issue at https://github.com/remix-run/remix/issues/9718

ehmicky commented 1 month ago

We're working on providing with a patch for this issue at https://github.com/sindresorhus/execa/pull/1141