workers-sdk icon indicating copy to clipboard operation
workers-sdk copied to clipboard

chore: consolidate `config` and `args`

Open threepointone opened this issue 3 years ago • 2 comments

There's a bit of tension in the way we read "configuration" right now.

  • We read config from a wrangler.toml file via the readConfig() function, which returns a Config object.
  • Ae also read configuration from cli args, which have similar (but not the same) names for fields. Notably, we don't apply validation to those cli args (except for some that we ask yargs to do, but not as extensively as readConfig does)
  • This isn't even totally true, since we do apply readConfig's validations to some fields. (Just legacy_env, and we do read the env name from --env)
  • Because of this tension, we end up having to pass data downstream in a spread out manner, instead of a single object.
  • This can be seen in publish' args, which is config + stuff to override config https://github.com/cloudflare/wrangler2/blob/5cc0772bb8c358c0f39085077ff676dc6738efd3/packages/wrangler/src/publish.ts#L29-L48
  • This is even worse with dev, which is not only a long list of props, but also has to be drilled through every component, whcih would otherwise have been a single config object in context https://github.com/cloudflare/wrangler2/blob/5cc0772bb8c358c0f39085077ff676dc6738efd3/packages/wrangler/src/dev/dev.tsx#L25-L53
  • I think the thing to do here, is field by field, make sure args values are being read and applied into the Config object
  • Then start removing those fields from being passed separately into dev and publish
  • So ideally (but probably not 100%), both dev and publish will take one config object, plus just a few more additional fields (if at all)

threepointone avatar Jun 26 '22 19:06 threepointone

💯

petebacondarwin avatar Jun 26 '22 20:06 petebacondarwin

I think the thing to do here, is field by field, make sure args values are being read and applied into the Config object

I started doing something like this with envName which I think could be a reference for how we could

https://github.com/cloudflare/wrangler2/blob/75f3ae829b0b4f8ae2cf2093bda93e8096838240/packages/wrangler/src/config/validation.ts#L118-L125

JacobMGEvans avatar Jun 27 '22 21:06 JacobMGEvans

Hey! 👋 We're hoping to solve a lot of these problems with the current Wrangler-as-a-library work we're doing. Specifically, we're planning to extract configuration handling into an isolated ConfigController component, that handles the merging of config between API options (or CLI flags), and the wrangler.toml file. This will allow us to standardise how we consolidate these options in the next major version. 👍

mrbbot avatar Nov 27 '23 16:11 mrbbot