Add "default" Option for "external" Environment Vars

[ObliviousHarmony]

There may be cases where a caller doesn't want to pass an "external" environment variable. In those cases, rather than relying on the script to handle the default case, wireit can support a "default" option. This option is used when no value is passed for an "external" variable.

[aomarks]

Hi @ObliviousHarmony! Could you elaborate a bit? I think I'm missing something. Maybe an example?

[ObliviousHarmony]

Thanks for the prompt reply @aomarks!

There are some cases where you may have tasks that are nearly identical except for an environment variable being different. This can be solved by using "external", however, if you do that your script either needs to handle a default case or any caller needs to set the environment variable. By setting a default you can remove the need for superfluous environment variables set in scripts when they aren't needed. As a more tangible example, here are three configuration files that all do the same thing when you run build and build:dev.

Separate Commands

{
	"scripts": {
		"build": "wireit",
		"build:dev": "wireit"
	},
	"wireit": {
		"build": {
			"command": "webpack",
			"env": {
				"NODE_ENV": "development"
			}
		},
		"build:dev": {
			"command": "webpack",
			"env": {
				"NODE_ENV": "production"
			}
		}
	}
}

Extra Command

{
	"scripts": {
		"build": "cross-env NODE_ENV=production npm run build:real",
		"build:dev": "cross-env NODE_ENV=development npm run build:real",
		"build:real": "wireit"
	},
	"wireit": {
		"build:real": {
			"command": "webpack",
			"env": {
				"NODE_ENV": {
					"external": true
				}
			}
		}
	}
}

With "default" Option

{
	"scripts": {
		"build": "wireit",
		"build:dev": "cross-env NODE_ENV=development npm run build"
	},
	"wireit": {
		"build": {
			"command": "webpack",
			"env": {
				"NODE_ENV": {
					"external": true,
					"default": "production"
				}
			}
		}
	}
}

---

At least in our use-case, the only difference in build scripts come from environment variables. In my opinion this feature makes the configuration easier to maintain without adding unnecessary commands and superfluous environment variables. This even becomes more tedious with extra environment variables that are sometimes set and sometimes not.

[ObliviousHarmony]

Any follow-up here @aomarks?

[aomarks]

I think your idea makes sense. So the 3 patterns are:

1. Use FOO in the fingerprint:

{
  "env": {
    "FOO": {
      "external": true
    }
  }
}

2. Set FOO to "bar" and use it in the fingerprint:

{
  "env": {
    "FOO": "bar"
  }
}

3. Set FOO to "bar", unless it's already set to something, and in either case use it in the fingerprint:

{
  "env": {
    "FOO": {
      "external": true,
      "default": "foo"
    }
  }
}

What would this case mean? It could either be an error, it could act like 2, or it could add like 3. I wonder if calling it "value" and making it act like 2 would make sense?

{
  "env": {
    "FOO": {
      "default": "foo"
    }
  }
}

[ObliviousHarmony]

When we consider that the object value of environment variables comes across as configuration options for it, I think a "value" option that behaves like "FOO": "bar" makes the most sense @aomarks. I'd be agreeable to this change.

[ObliviousHarmony]

I took another look today and "value" feels a little confusing. The name "value" when paired with "external" doesn't have as clear a meaning as "default". At the same time, "default" without "external" also isn't clear because it isn't so much a default as it is the value.

I actually think it might be better to provide an error for #3 @aomarks. Since this PR currently provides an error in that way, I'm happy with this PR as-is.

[ObliviousHarmony]

Any thoughts @aomarks?

[aomarks]

I took another look today and "value" feels a little confusing. The name "value" when paired with "external" doesn't have as clear a meaning as "default". At the same time, "default" without "external" also isn't clear because it isn't so much a default as it is the value.

I actually think it might be better to provide an error for #3 @aomarks. Since this PR currently provides an error in that way, I'm happy with this PR as-is.

Yeah, that makes sense to me. And I agree that this code will already error in the way we discussed. Thanks for the PR, merging!

[ObliviousHarmony]

I merged trunk to fix the CI issues @aomarks.