Follow up to #3420

[samtstern]

Description

Follow up to #3420

• Do a first modernization pass on config.ts
• Improve the typing of as many options.config.get("...") calls in *.ts files as I easily could

Scenarios Tested

Relying on compiler and unit tests here.

Sample Commands

N/A

[inlined]

Generally LGTM except:

1. There's a good handful of new casts in here. Why are they needed?
2. I'm nervous that the functions default directory needs to be checked in all places for a fallback to default. Why can't we just put the default in config?
3. I already didn't understand the difference between options and options.config. Now we have options.config and options.config.src. Do we really need the "src" bag?

[inlined]

Why do we need a cast here?

[samtstern]

Previously options was any so options.project was also any. Now options is Options so options.project is unknown. We have to assert the type as string. It's not type-safe, but it wasn't before either. The slight improvement here (IMO) is putting the ugly cast out in the open.

I'll change this to use an assertion instead.

[samtstern]

@inlined responses to your comments:

There's a good handful of new casts in here. Why are they needed?

Now that options is not any it has a lot of unknown properties. We were always assuming their types before, now the compiler is just making us reveal our shame with casts. I changed a lot of the casts to assertions now, which I think is better?

In general this codebase has a bad pattern where things are bolted on to options over time and then modules make assumptions about what has run before them. For example options.user is only there after certain auth utilities run. options.project is only there after requireProject.

Removing that pattern is the right thing to do but also very daunting.

I'm nervous that the functions default directory needs to be checked in all places for a fallback to default. Why can't we just put the default in config?

I would love a better way for us to handle defaults. Right now the default is set with a config.set() at runtime so the compiler doesn't know about it. What do you think is the most elegant way to handle this?

I already didn't understand the difference between options and options.config. Now we have options.config and options.config.src. Do we really need the "src" bag?

Can you suggest a clean way to get rid of it?

[inlined]

@inlined inlining responses to inlined responses to @inlined's comments

Now that options is not any it has a lot of unknown properties. We were always assuming their types before, now the compiler is just making us reveal our shame with casts. I changed a lot of the casts to assertions now, which I think is better?

I guess I just didn't understand at first why we can't have things like an explicit project: string | undefined. I feel better about null assertions than type assertions (not great about either, but that's a separate discussion)

In general this codebase has a bad pattern where things are bolted on to options over time and then modules make assumptions about what has run before them. For example options.user is only there after certain auth utilities run. options.project is only there after requireProject.

I've contemplated these issues a few times and I agree that I don't know the right solution. In Functions' code I just comment each field as the phase in which it becomes available, but I wish there were an elegant way to upcast a PrepareOptions to a DeployOptions to a ReleaseOptions so things would go from nullable to defined.

I would love a better way for us to handle defaults. Right now the default is set with a config.set() at runtime so the compiler doesn't know about it. What do you think is the most elegant way to handle this?

Why does default initialization in the Config not work?

Can you suggest a clean way to get rid of [Options.config.src]?

Maybe I'm being dense here, but why is it hard to unnest an object? Why is src valuable today? Maybe it is better to wait until Options.config.get/set is removed entirely and then just remove the src bag everywhere?

[samtstern]

I guess I just didn't understand at first why we can't have things like an explicit project: string | undefined. I feel better about null assertions than type assertions (not great about either, but that's a separate discussion)

We can do this for the options which are present on every command like project and debug. For the per-command options I think we'll need a better system than defining them all as optional on this one type. I'll add the ones I can.

Why does default initialization in the Config not work?

Does TS support a default value in an interface? I don't think so. Right now the FirebaseConfig file is meant to represent the contents of firebase.json, not the contents + implied defaults.

I guess I could make a new type with the defaults and merge them? I do think it's important to have a type which represents the JSON config exactly as provided by the developer.

Maybe I'm being dense here, but why is it hard to unnest an object? Why is src valuable today? Maybe it is better to wait until Options.config.get/set is removed entirely and then just remove the src bag everywhere?

This was my plan, yes. But I can unnest it now if you want.

[samtstern]

@inlined it's actually not safe to unnest src onto config until we get rid of config.set() since that modifies data which is like a bizzaro parallel word of src ...

[inlined]

Sorry, I thought I had LGTM'd earlier. I don't know if I agree that we need a type that matches the JSON directly as opposed to w/ defaults, but I'm not going to make a huge deal about it. And I understand that src can't be deleted yet. It would be nice if we could eventually get there though.