define env type

[holic]

fixes #1132

I've seen the issue described in #1132 happen in both Remix and Next.js projects

[ehmicky]

Hi @holic,

Thanks for this PR.

The issue at #1132 was specific to Remix. It is due to a bug in Remix currently tracked in remix-run/remix#9718. It impacts node:child_process, and is not specific to Execa. So there's nothing to fix in Execa.

When it comes to Next.js, it seems like they basically have the same bug. I reported it in vercel/next.js#68749, but their issue tracking automation closed it automatically because I used a TypeScript playground link instead of creating a GitHub repository.

[ehmicky]

So basically the core problem is this: the following pattern is bad.

// Augment `ProcessEnv` type from Node.js
declare global {
  namespace NodeJS {
    interface ProcessEnv { 
      readonly NODE_ENV: "development" | "production" | "test"; 
    }
  }
} 

The intent is to change the type of process.env.NODE_ENV, which is fair. However, ProcessEnv is also used by Node.js to type the env option of node:child_process spawn(). So the above code creates unintentional problems.

Unfortunately, this pattern was introduced by Next.js, which was seemingly copied by Remix, and might have been copied by other libraries.

This is not a bug with Execa. However, getting that bug fixed in either Next.js or Remix seems difficult. So I would suggest for Execa to patch that typing problem, since that patch seems rather simple.

From that perspective, your solution looks good. However, could you please add some type tests? Specifically, a type test that does the interface Process { ... } augmentation, then calling Execa with the env option, and make sure the types work in that case. Thanks!

For example, the following type tests fail without your PR, but succeed with it.

import process, {type env} from 'node:process';
import {expectType, expectAssignable} from 'tsd';
import {execa, type Options, type Result} from '../../index.js';

type NodeEnv = 'production' | 'development' | 'test';

// Libraries like Next.js or Remix do the following type augmentation.
// The following type tests ensure this works with Execa.
// See https://github.com/sindresorhus/execa/pull/1141 and https://github.com/sindresorhus/execa/issues/1132
declare global {
	// eslint-disable-next-line @typescript-eslint/no-namespace
	namespace NodeJS {
		// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
		interface ProcessEnv {
			readonly NODE_ENV: NodeEnv;
		}
	}
}

// The global types are impacted
expectType<NodeEnv>(process.env.NODE_ENV);
expectType<NodeEnv>('' as (typeof env)['NODE_ENV']);
expectType<NodeEnv>('' as NodeJS.ProcessEnv['NODE_ENV']);
expectType<NodeEnv>('' as globalThis.NodeJS.ProcessEnv['NODE_ENV']);

// But Execa's types are not impacted
expectType<string | undefined>('' as Exclude<Options['env'], undefined>['NODE_ENV']);
expectAssignable<Result>(await execa({env: {test: 'example'}})`unicorns`);
expectAssignable<Result>(await execa({env: {test: 'example'} as const})`unicorns`);
expectAssignable<Result>(await execa({env: {test: undefined}})`unicorns`);
expectAssignable<Result>(await execa({env: {test: undefined} as const})`unicorns`);

[holic]

I agree the original pattern and source of this issue isn't great (haven't we learned enough that fiddling with globals is a bad idea?) but seems easy to simplify types here while maintaining intended use and compatibility.

Happy to add some tests! Will do in the next day or so.

[holic]

Added! Thanks for making that an easy copy+paste job for me :)

[ehmicky]

Looks good to me, except the one small suggestion above.

To be more explicit, the previous type of options.env was:

{ 
  TZ?: string;
  [key: string]: string | undefined;
}

And the new type is:

{ 
  readonly [key: string]: string | undefined;
}

This is backward compatible with the former one, i.e. this is not a breaking change.

@sindresorhus What do you think?

[sindresorhus]

👍

[ehmicky]

Thanks @holic!