Add strong typing to context, options, and paylaod in Functions codebase #3271
Conversation
There was a problem hiding this comment.
This looks like a really good direction to go in, and I really like the idea of moving things out of context into caching helper functions. Bryan and Sam may also be interested in this, they've been talking about trying to type context/options/payload across the whole codebase for a while now
| @@ -51,15 +52,19 @@ export async function checkServiceAccountIam(projectId: string): Promise<void> { | |||
| * @param options The command-wide options object. | |||
| * @param payload The deploy payload. | |||
| */ | |||
| export async function checkHttpIam(context: any, options: any, payload: any): Promise<void> { | |||
| const functionsInfo = payload.functions.triggers; | |||
| export async function checkHttpIam( | |||
There was a problem hiding this comment.
Not something that should be changed in this PR, but something I'm curious about your opinion on: what do you think about refactoring this function (and a couple other helper functions with similar signatures) to take just what it needs as params, instead of the entire context, options, payload?
I feel like it would be easier to understand and use this function if its signature was something like checkHttpIam(projectId: string, httpFunctionNames: string, existingFunctions: CloudFunction[], filters: string)
There was a problem hiding this comment.
It would also let us avoid these ugly non-null assertions that keep popping up
There was a problem hiding this comment.
I have mixed feelings. I've gotten used to "context' being passed around in Go and I prefer editing Context as the only side-effect allowed in our API calls. I've got no love for "Options' though since the actual command line flags passed seem like too low level a concern for this code (e.g. options' project vs context's project). And I have yet to find the purpose of Payload.
There was a problem hiding this comment.
Agree with you on options and (especially) payload. We use this same "context, options, paylod" pattern for all of the deploy code paths, and payload might get used more in the other deploy modules (hosting, storage rules, etc), but its basically unused here.
For context, I've bumped into a bunch of subtle bugs over my time with this codebase caused by using context as a mutable "global" state. I also find that it can make the code hard to follow sometimes - its not always obvious where context.blah was originally set or if it has been set at all. Having said that, I do think that your proposed changes (stronger typing, and moving complex objects like 'existingFunctions' out of context) alleviate these concerns a bit.
src/deploy/functions/args.ts
Outdated
| export interface Options { | ||
| cwd: string; | ||
| configPath: string; | ||
| // Why is there options.project and context.projectId? |
There was a problem hiding this comment.
options.project is the value of the --project/-P flag, context.projectId is the project Id that is actually in use, determined by https://github.com/firebase/firebase-tools/blob/master/src/getProjectId.js
There was a problem hiding this comment.
So should I make sure I change code that refers to options.project to context.projectId?
There was a problem hiding this comment.
Yeah, as far as I know, we pretty much should never use options.project outside of https://github.com/firebase/firebase-tools/blob/master/src/getProjectId.js
| force: boolean; | ||
| } | ||
|
|
||
| export interface FunctionsSource { |
There was a problem hiding this comment.
Nit: Should this live in prepareFunctionsUpload instead? This feels to me less like an inherent feature of the Context and more of a detail of prepareFunctionsUpload
There was a problem hiding this comment.
Hmm... interesting point. In my work-in-progress branch I have backend.existingBackend(context). Maybe I could do prepareFunctionsUpload.functionsSource(context)? I don't know that I want to have functionsSource: prepareFunctionsUpload.FunctionsSource because it'll create a weird dependency cycle.
How do you feel? (also, as an aside, I hate that we have file names named after a command, yet they act as a namespace for multiple things. Go-style namespacing ftw IMO)
| // line up. But it just so happens that we don't hit any bugs with the current | ||
| // implementation of the function. This will all go away once everything uses | ||
| // backend.FunctionSpec. | ||
| (context.existingFunctions! as any) as deploymentPlanner.CloudFunctionTrigger[], |
There was a problem hiding this comment.
TIL that you can totally circumvent Typsecript by casting to any then to the type you want
src/deploy/functions/validate.ts
Outdated
| * @param sourceDirName Relative path to source directory. | ||
| * @throws { FirebaseError } Functions directory must exist. | ||
| */ | ||
| export function functionsDirectoryExists(options: object, sourceDirName: string): void { | ||
| export function functionsDirectoryExists(options: { cwd: string }, sourceDirName: string): void { |
There was a problem hiding this comment.
Should this be options: { cwd?: string, configPath?: string}, like what resolveProjectPath takes? Its not immediately clear to me if this will only ever have cwd, or if it will have configPath depending on the options passed in.
There was a problem hiding this comment.
I'm a bit confused. Are you saying that sourceDirName should be part of the passed options? This is always an args.Options, which always has a cwd. I just changed it from args.Options because tests were annoying to fix if we required a full args.Options.
There was a problem hiding this comment.
Not quite - functionsDirectoryExists calls resolveProjectPath, which has the following signature: export function resolveProjectPath( options: { cwd?: string; configPath?: string }, filePath: string ):
I'm wondering if theres any edge cases where options.configPath would be defined here - from https://github.com/firebase/firebase-tools/blob/master/src/command.ts#L237, it looks like it comes from the --config flag.
There was a problem hiding this comment.
Documenting the configPath option. Making it optional since the existing tests don't use it, though it should always be present per my research into the Options type. Since both prod and test provide a CWD I'm leaving that as required.
…ase (firebase#3271) What it says in the box. The nebulous context: any, options: any, payload: any are now strongly typed. To get part of this working I had to convert strings to Runtimes. I also removed a few lodash calls that I saw.
What it says in the box. The nebulous
context: any, options: any, payload: anyare now strongly typed.To get part of this working I had to convert strings to Runtimes. I also removed a few lodash calls that I saw.