Feat/973 provide configuration via env vars

[pafik13]

Fixes #973

  process.env.ZX_VERBOSE = 'true'
  const envVariables = extractFromEnv()
  envVariables.verbose /// <- here should be Boolean

• Tests pass
• Appropriate changes to README are included in PR

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[antongolub]

1. I'd suggest getZxDefaults
2. Let's extend signature for more convenient testing: (defs: Options, prefix: string = 'ZX_', env = process.env)
3. The reduce API might be better here:

return Object.entries(env).reduce((m, [k, v]) => {
  if (k.startsWith(prefix)) {
    const _k = snakeToCamel(key.slice(prefix.length))
    const _v = ({true: true, false: false})[v.toLowerCase()] ?? v
    if (typeof _v === typeof defs[_k])
      m[_k] =  _v
  }
  return m
}, defs)

// core.ts
const defaults = getZxDefaults({
  [CWD]:          process.cwd(),
  [SYNC]:         false,
  verbose:        false,
  //..
})

[pafik13]

Looks pretty nice. Thank you. I will rewrite it ASAP)

There is only one thing that worries me in default value type checking: mixed types. For example, shell and preferLocal. In defaults it a boolean, but we can pass a string programmatically. This condition rejects such values.

What do you think about this? @antongolub

[pafik13]

I updated code with some potentially improvements:

1. I iterate through defaults cause it contains less keys then env (i hope)))
2. I use parameter extra to determine optional fields types and additional type for localPrefer

[antongolub]

k.startsWith(prefix) is cheap, so its fine to traverse the whole env

[antongolub]

We can avoid extra in arguments. Fn internal constant is fine here:

const types = {
   shell: ['string', 'boolean']
}

if ([typeof defs[_k], ...types[_k]].includes(typeof _v))

[antongolub]

Hey, @pafik13,

1. I'm afraid, no chance to land w/o CLA sign
2. type assertions against the default map values are necessary.

[antongolub]

Let's keep the original formatting here for git blame

[antongolub]

// prettier-ignore
export const defaults: Options = getZxDefaults({
  // ... to keep git blame
})

[pafik13]

I restored formatting, but it didn't help (:

[antongolub]

prettier-ignore should go above the export const defaults declaration

[pafik13]

It was there and extended only to the closest expression - the function call...
JSON declaration was out of scope)
Now, JSON isn't formatted

[pafik13]

It was hard, but I did it.
Thank you)

[pafik13]

[antongolub]

// prettier-ignore
export const defaults: Options = getZxDefaults({
  // ... to keep git blame
})

[antongolub]

{...undefined}
undefined is spreadable, afair, isnt it?

[pafik13]

I thought the same, but output from my Browser console

[...undefined]
VM153:1 Uncaught TypeError: undefined is not iterable
    at <anonymous>:1:5

[pafik13]

but in your case it works)

> {...undefined}
< {}

[antongolub]

Minor optimization: v && k.startsWith(prefix)

[antongolub]

I guess we should restrict the accepted env opts: sync, nothrow, input, halt, stdio can break the script logic, while verbose, shell, quiet, preferLocal just change its behaviour. Moreover, timeout does not belong to defaults, but it looks reasonable to set externally.

const allowed = new Set(['timeout', 'shell', ...])
// ...
if (allowed.has(_k)) {
}

And if somebody is setting ZX_CWD=true, wrong typecast to bool is the least of problems. So maybe we can skip the type check at all

[pafik13]

Plz clarify what you suggest:

• determine only one list of acceptable properties (verbose, shell, quiet, preferLocal) and try it extract from env with type-checking

• determine two lists:

  • acceptable - extract w/o type check
  • unacceptable - extract with strict type check

[antongolub]

How about this?

(defs: Options, prefix = 'ZX_', env = process.env) => Object.entries(env).reduce<Options>((m, [k, v]) => {
  const allowed = new Set(['preferLocal', 'detached', 'verbose', 'quiet', 'timeout', 'timeoutSignal', 'prefix', 'postfix'])
  if (v && k.startsWith(prefix)) {
    const _k = snakeToCamel(key.slice(prefix.length))
    const _v = ({true: true, false: false})[v.toLowerCase()] ?? v
    if (allowed.has(_k))
      m[_k] =  _v
  }
  return m
}, defs)

[pafik13]

You insistently suggest pass Options to reduce method - it isn't work in this place m[_k] = _v. Since _k is random key of Options, m[_k] has type never as union of string, boolean, StdioInput and other values of Options.

[pafik13]

How about this?

(defs: Options, prefix = 'ZX_', env = process.env) => Object.entries(env).reduce<Options>((m, [k, v]) => {
  const allowed = new Set(['preferLocal', 'detached', 'verbose', 'quiet', 'timeout', 'timeoutSignal', 'prefix', 'postfix'])
  if (v && k.startsWith(prefix)) {
    const _k = snakeToCamel(key.slice(prefix.length))
    const _v = ({true: true, false: false})[v.toLowerCase()] ?? v
    if (allowed.has(_k))
      m[_k] =  _v
  }
  return m
}, defs)

In that case we can assign to verbose and quiet some string value. It's not good, in my opinion.
So, I combined 'allowed' and type-check together into types and use only it.

[antongolub]

Nice! Thanks for the contribution.

[pafik13]

Nice! Thanks for the contribution.

Thanks for the mentorship)