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

Add environment variable configuration #581

Merged
merged 6 commits into from Dec 6, 2022
Merged

Add environment variable configuration #581

merged 6 commits into from Dec 6, 2022

Conversation

aomarks
Copy link
Member

@aomarks aomarks commented Dec 3, 2022

Adds the ability to either set environment variables directly, or to indicate that an externally-provided environment variable affects the behavior of the script.

Setting variables directly is useful because there is no built-in way to do this in a cross-platform way (especially macOS/Linux shells vs Windows shells). A typical solution is to use https://github.com/kentcdodds/cross-env (which is deprecated), but now wireit can take over this responsibility.

Indicating that an external environment variable affects the behavior of a script is useful because the value will be included in the fingerprint, meaning output will not be re-used if that value changed.

Example:

{
  "wireit": {
    "bundle:prod": {
      "command": "rollup -c",
      "files": ["lib/**/*.js", "rollup.config.js"],
      "output": ["dist/bundle.js"],
      "env": {
        "MODE": "prod",
        "DEBUG": {
          "external": true
        }
      }
    }
  }
}

Fixes #93

if (envNode === undefined) {
return {};
}
if (command === undefined) {

Choose a reason for hiding this comment

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

Does this mean passthrough commands can't set env vars?

What about a pair of start and start:dev scripts that are both pass-throughs where the only difference is that start:dev sets something like MODE: "development" for other scripts?

Copy link
Member Author

@aomarks aomarks Dec 3, 2022

Choose a reason for hiding this comment

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

Does this mean passthrough commands can't set env vars?

Yep. In general env only applies to that specific script, it doesn't apply transitively, so it doesn't make sense if there is no command, since it would have no effect. (The non-transitiveness is documented in the README).

What about a pair of start and start:dev scripts that are both pass-throughs where the only difference is that start:dev sets something like MODE: "development" for other scripts?

In that case I'd create two different branches of the dependencies. You'd probably want different output folders anyway.

Choose a reason for hiding this comment

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

Hmm, maybe I should reopen #556

Because my question there is about how to avoid having a big duplication of scripts to handle the cross-cutting prod/dev cases. If I have separate scripts, then I don't have any need for an env variable to be an input to the fingerprint - I would either not use env vars, or each script would only ever be invoked with a single value of the variable.

Choose a reason for hiding this comment

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

Another case where sharing env vars would be important is if we figure out how to start services with dynamic resources - like a server with a dynamic port. In that case we'd like to somehow extract the port and give it to anything that depends on it. That would allow for starting services multiple times (the opposite of what I'm asking for in #580, I think both behaviors would be useful).

This is two different directions of sharing: from dependent->dependency, and from dependency->dependent.

Taking just the dependent->dependency which I'm asking about originally, could we have a way to explicitly share the value via a property in dependencies?

{
  "build" {
    "command": "...",
    "env": {
      "MODE": "external"
    }
    "files": [],
    "output": [],
  },
  "start": {
    "command": "...",
    "env": {
      "MODE": {
        "external": true,
    },
    "dependencies": {
      "build": {
        "env": {
          "MODE": {
            // pass "start"s MODE value to "build"
            "inherit": true,
          }
        }
      }
    }
  }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then any dependency of the dependency would also need the env option too if you need it to propagate. Would it be a bad idea to add something like "cascade" at the script's env config, where it could be like this?

{
  "start": {
    "command": "...",
    "env": {
      "MODE": {
        "value": "prod",
        "cascade": true
      }
    }
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

If an environment variable applies to all dependencies, then that implies that that environment variable should be included in the fingerprint. But very often that's not desirable, because it would mean you can't re-use output even though the variable doesn't actually matter for some particular script.

For example, if you had a dev and prod rollup script which differ via an environment variable., but both of them consumed the same tsc output, then you'd lose the cache hit for the tsc step.


Things also get a bit confusing when you have multiple paths to the same dependency, e.g. imagine this graph:

   A
  / \
 v   v
 B   C [BUILD=prod]
  \ /
   v
   D

So now D as traversed via B has BUILD=undefined, but as traversed via C has BUILD=prod. It's not clear to me what we should do there, and I suspect the answer is we shouldn't get ourselves in that situation to begin with.

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed some more offline with Justin -- I had actually missed the comment about setting "inherit": true, which addresses my concerns above pretty much. We also extended the idea to consider how you could directly reference the specific script where you want to inherit an environment variable from. Also agreed that this can be a new extension to this feature, so we can land this initial support now.

Copy link
Collaborator

@augustjk augustjk left a comment

Choose a reason for hiding this comment

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

Awesome!

@aomarks aomarks merged commit bd84f60 into main Dec 6, 2022
@aomarks aomarks deleted the env branch December 6, 2022 22:05
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 this pull request may close these issues.

Ability to specify which environment variables should affect cache key
3 participants