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

Unable to extend env when ProcessEnv has been manipulated #1132

Closed
ehyland opened this issue Jun 26, 2024 · 7 comments · Fixed by #1141
Closed

Unable to extend env when ProcessEnv has been manipulated #1132

ehyland opened this issue Jun 26, 2024 · 7 comments · Fixed by #1141

Comments

@ehyland
Copy link

ehyland commented Jun 26, 2024

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
Copy link
Collaborator

ehmicky commented Jun 27, 2024

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 (remix-run/remix#983).

I would suggest one of the following:

  • Posting an issue on @remix-run to make NODE_ENV optional. They probably need to use both ? and ... | undefined to follow how Node.js types ProcessEnv.
  • Adding env.NODE_ENV to your code.
  • Workaround using 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
Copy link
Collaborator

ehmicky commented Jul 2, 2024

@ehyland Did this fix your issue?

@ehyland
Copy link
Author

ehyland commented Jul 3, 2024

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
Copy link
Collaborator

ehmicky commented Jul 3, 2024

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.

@ehmicky ehmicky closed this as completed Jul 3, 2024
@ehyland
Copy link
Author

ehyland commented Jul 4, 2024

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

@ehmicky
Copy link
Collaborator

ehmicky commented Jul 5, 2024

I created an issue at remix-run/remix#9718

@holic holic mentioned this issue Aug 9, 2024
@ehmicky
Copy link
Collaborator

ehmicky commented Aug 9, 2024

We're working on providing with a patch for this issue at #1141

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants