Parse Firebase extensions from code #1590
Conversation
| (cd "$f" && npm link @firebase-extensions/firebase-firestore-translate-text-sdk) | ||
| (cd "$f" && npm link @firebase-extensions/local-backfill-sdk) |
There was a problem hiding this comment.
instead of adding extensions "functions" as a test case on every single set up, can we instead separate out extensions test cases as unique ones?
There was a problem hiding this comment.
But it's the same if-else code chunk being tested... I don't want someone to "forget" to run half the tests.
There was a problem hiding this comment.
I don't think anybody runs this test individually but instead runs it via this test script.
I think it'd be cleaner to separate out extension use case - I see it as a specialized use case and isn't representative of most CF3 users set up.
| @@ -99,14 +189,16 @@ export function mergeRequiredAPIs(requiredAPIs: ManifestRequiredAPI[]): Manifest | |||
| export async function loadStack(functionsDir: string): Promise<ManifestStack> { | |||
| const endpoints: Record<string, ManifestEndpoint> = {}; | |||
| const requiredAPIs: ManifestRequiredAPI[] = []; | |||
| const extensions: Record<string, ManifestExtension> = {}; | |||
There was a problem hiding this comment.
can extensions just be undefined if we don't have any extensions?
that feels like a safer, backward compat change (just introduce new key whenever you are defining the new extensions functions)
There was a problem hiding this comment.
I tested to make sure that detectFromYaml and detectFromPort handle "extra" things by ignoring them so this change is backwards compatible as is. Technically, endpoints and extensions are at the same level. We could have extensions and no endpoints, endpoints and no extensions, or both, or neither. I'd like that to be reflected in the code. They should be handled the same way.
There was a problem hiding this comment.
Do we think defining just the extensions as a valid use case? Not questioning whether it's technically valid - I just didn't think it's a recommended or even endorsed way to manage and deploy extensions.
Shouldn't you be using extensions manifest in your firebase.json to do that?
And this addition in Functions SDK is mainly to provide a way to easily write custom event handler as CF3 functions to extend your extensions?
There was a problem hiding this comment.
Yes, it's a valid use case. This is not just for functions defined for extensions. It's the new way to define extensions apart from the manifest. It's useful for handling multiple similar extensions as well.
My original plan was not to involve functions code at all and have all the extensions definitions handled by extensions code, but Thomas and Michael insisted that the new extensions definitions MUST live in the functions codebase and MUST be parsed by the functions SDK. It made everything much more complicated because extensions can now be deployed using firebase deploy --only functions (so complexity x 10 as there are now multiple places to deploy extensions and they must not clobber each other etc.) Which I thought was super confusing. But Thomas insisted and said he would do "everything in his power" to prevent me from doing it any other way. So here we are :)
src/runtime/manifest.ts
Outdated
| * @alpha | ||
| */ | ||
| export interface ManifestStack { | ||
| specVersion: "v1alpha1"; | ||
| params?: WireParamSpec<any>[]; | ||
| requiredAPIs: ManifestRequiredAPI[]; | ||
| endpoints: Record<string, ManifestEndpoint>; | ||
| extensions: Record<string, ManifestExtension>; |
There was a problem hiding this comment.
same here - make extensions undefined-able (extensions?)
There was a problem hiding this comment.
Same as above. It's already backwards compatible etc. If extensions are made undefined-able then the same should be true for endpoints... I don't want people thinking "We always have functions, but only sometimes have extensions" because it's not true
There was a problem hiding this comment.
Understand your point, but I see extensions use case for the Functions SDK as an "extensions" to the Functions SDK, not the core experience of using the Functions SDK.
For instance, both go and dart implementation for "Firebase Functions SDK" relied on writing on the functions.yaml by hand instead. In those cases, the functions.yaml definition we defined in the Node.js served as the schema definition.
By making extensions a "required" field, we would be making these functions.yaml to add:
extensions: {}
I don't like that experience since it would be surprising to have to write extensions on their functions manifest.
(Granted, we don't really expect developers to write functions.yaml by hand, but we are talking about schema definition, and I don't think extensions should be necessary)
There was a problem hiding this comment.
I agree that it's very surprising to have extensions mixed in with the functions. I made that argument strenuously and Thomas insisted that they must be. And since it must be, then we should be accurate and have extensions mixed in with the functions everywhere like he wanted. Especially since we might also support Go and dart for extensions at some point.
| "_EVENT_ARC_REGION": "us-central1", | ||
| "_FUNCTION_LOCATION": "us-central1", | ||
| }); | ||
| exports.extRef1 = extRef1; |
There was a problem hiding this comment.
nit* why not
exports.extRef1 = firestoreTranslateText("extRef1", {
"COLLECTION_PATH": "collection1",
"INPUT_FIELD_NAME": "input1",
"LANGUAGES": "de,es",
"OUTPUT_FIELD_NAME": "translated",
"_EVENT_ARC_REGION": "us-central1",
"_FUNCTION_LOCATION": "us-central1",
});
There was a problem hiding this comment.
Done for the ones not referenced by functions. Also removed the extensions from the parameters test.
Parse Firebase extensions from code