Moves most of https pacakge into /common/providers/https.#915
Moves most of https pacakge into /common/providers/https.#915
Conversation
The actual encoding/decoding/error logic etc. of callable functions will stay the same between v1 and v2, so they should be in a /common file. The rest of /v1 is about trigger options, cors, etc. onCall's CORS options are part of the v1 namespace because v2 adds a "cors" option to HttpsOptions. In the v2 API, we will default to `"cors": true`, but a customer could say `"cors": "my.firebase.app"` and we will respsect it. I wish there was a way to make this diff not so noisy. Most of this is cut & paste.
mbleigh
left a comment
There was a problem hiding this comment.
There are some changes needed, but nothing that I have to see the result of before submitting.
src/common/proto.ts
Outdated
| } | ||
| } | ||
|
|
||
| export function renameIfPresent<Src, Dest>( |
There was a problem hiding this comment.
How about transformIfPresent or convertIfPresent since your use here with failurePolicy doesn't actually rename the field.
| }); | ||
|
|
||
| it('should not set a serviceAccountEmail if service account is set to `default`', () => { | ||
| it('should set a null serviceAccountEmail if service account is set to `default`', () => { |
There was a problem hiding this comment.
Is this correct? If serviceAccountEmail is null (vs. not set on the wire) does that imply something different?
There was a problem hiding this comment.
The previous behavior did nothing if the user specified "default". We dropped it on the floor and never restored the default on the backend. Using null causes us to specify the field in our update mask and causes the backend to correctly restore the default on UpdateFunction.
| } | ||
|
|
||
| // Returns true if req is a properly formatted callable request. | ||
| function isValidRequest(req: Request): req is HttpRequest { |
There was a problem hiding this comment.
Please re-add @hidden on all unexported functions, since typedoc is bad.
src/common/providers/https.ts
Outdated
| } | ||
|
|
||
| /** @internal */ | ||
| export let onCallHandler = ( |
There was a problem hiding this comment.
Is there a reason for export let vs. export const? Also why not just export function?
Lastly, I wonder if this should be a factory instead onCallHandler({cors: Corsoptions}), that way a critical piece of functionality (making the request pass cors) can't be accidentally forgotten in a future version.
There was a problem hiding this comment.
export const is correct. I used lambadas instead of functions because only the former supports currying syntax. I'll make another helper that does this correctly with CORS.
| dest[destField] = converter(src[srcField]); | ||
| } | ||
|
|
||
| export function serviceAccountFromShorthand( |
There was a problem hiding this comment.
This doesn't really have anything to do with protos, so I wonder if this should be elsewhere or the file should be named something else.
There was a problem hiding this comment.
Will make this something like encoding.ts. Just used proto.ts to copy a poorly named but much beloved grab bag in the CLI.
…nctions into inlined.common-http
Builds upon #915 Creates the first bits of v2/ setGlobalOptions allows customers to set options that will be preserved across any function written in the v2 API. v2/https has webhook and callable function support. In addition to previous features, customers can now specifiy `cors` as a shorthand for using CORS middleware. We will probably eventually support scheduled functions in the HTTPS namespace but that will have to wait for another day.
Builds upon #915 Creates the first bits of v2/ setGlobalOptions allows customers to set options that will be preserved across any function written in the v2 API. v2/https has webhook and callable function support. In addition to previous features, customers can now specifiy `cors` as a shorthand for using CORS middleware. We will probably eventually support scheduled functions in the HTTPS namespace but that will have to wait for another day.
Builds upon #915 Creates the first bits of v2/ setGlobalOptions allows customers to set options that will be preserved across any function written in the v2 API. v2/https has webhook and callable function support. In addition to previous features, customers can now specifiy `cors` as a shorthand for using CORS middleware. We will probably eventually support scheduled functions in the HTTPS namespace but that will have to wait for another day.
* Moves most of https pacakge into /common/providers/https. The actual encoding/decoding/error logic etc. of callable functions will stay the same between v1 and v2, so they should be in a /common file. The rest of /v1 is about trigger options, cors, etc. onCall's CORS options are part of the v1 namespace because v2 adds a "cors" option to HttpsOptions. In the v2 API, we will default to `"cors": true`, but a customer could say `"cors": "my.firebase.app"` and we will respsect it. I wish there was a way to make this diff not so noisy. Most of this is cut & paste. * Finish refactor of HTTPS code Builds upon #915 Creates the first bits of v2/ setGlobalOptions allows customers to set options that will be preserved across any function written in the v2 API. v2/https has webhook and callable function support. In addition to previous features, customers can now specifiy `cors` as a shorthand for using CORS middleware. We will probably eventually support scheduled functions in the HTTPS namespace but that will have to wait for another day. * Export logger * run formatter * Revert pacakge-lock.json
…tained a function (#1000) In #915, we replaced use of `_.isObject(data)` with `typeof data === 'object'`. Turns out that `_.isObject` returns `true` for functions ([code](https://github.com/lodash/lodash/blob/2da024c3b4f9947a48517639de7560457cd4ec6c/isObject.js#L26)). We update the condition to match the previous behavior. Fixes #964
The actual encoding/decoding/error logic etc. of callable functions
will stay the same between v1 and v2, so they should be in a /common
file.
The rest of /v1 is about trigger options, cors, etc.
I tried to get GitHub to pick up that most of this change is a cut + paste, but it doesn't seem to want to take the hint. We may just need to trust that much of the code can be glossed over.
onCall's CORS options are part of the v1 namespace because v2 adds
a "cors" option to HttpsOptions. In the v2 API, we will default
to
"cors": true, but a customer could say"cors": "my.firebase.app"and we will respect it.
I started this change while writing the v2 namespace as a whole and pulled in a few diffs from that. One notable change in behavior is that we set
__trigger.serviceAccounttonullwhen the optiondefaultwas provided. This is a bugfix because previously settingdefaultwould do nothing to__trigger.serviceAccountand we wouldn't ever update the function accordingly on deploy.firebase-toolsnow has meaningful nulls and settingserviceAccountto null will cause it to be included in the update field mask, which will cause the value to be reset to the server-side default.