Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finish refactor of HTTPS code #917

Merged
merged 5 commits into from
Jul 19, 2021
Merged

Finish refactor of HTTPS code #917

merged 5 commits into from
Jul 19, 2021

Conversation

inlined
Copy link
Member

@inlined inlined commented Jul 15, 2021

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.

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.
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.
@inlined inlined requested a review from joehan July 15, 2021 16:59
"./v2/options": "./lib/v2/options.js",
"./v2/https": "./lib/v2/providers/https.js"
},
"typesVersions": {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the submodule exports for typescript. TS imports don't actually support submodule exports yet, but typesVersion allows us to do a rewrite of the lookup paths for .d.ts files. The Admin and Web SDK use this hack.

Copy link
Contributor

@joehan joehan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good to me - but why did you get rid of the entire packages section of package-lock.json?

/**
* List of all regions supported by Cloud Functions v2
*/
export const SUPPORTED_REGIONS = ['us-west1'] as const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about const assertions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we can blame lauren for this one. I just copied the existing code from v1

*/
export function setGlobalOptions(options: GlobalOptions) {
if (globalOptions) {
logger.warn('Calling setGlobalOptions twice leads to undefined behavior');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually true? Looks pretty clear that it just overwrite all previous global options.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undefined doesn't mean unpredictable. I just want to set expectations that this is not an OK thing to do. The warning might also be interesting if someone is surprised that it's being set twice. E.g. If they have multiple files they're importing which each set global options. Or if some malicious codebase decided it would be funny to call setGlobalOptions({minInstances: 100})

@inlined
Copy link
Member Author

inlined commented Jul 16, 2021

RE removing the packages section of pacakge-lock.json, this just happened because I ran npm install and it overwrote the package-lock.json with a v2 format. I can revert for now, but we'll have to face the v2 upgrade once we add new deps.

@joehan
Copy link
Contributor

joehan commented Jul 19, 2021

Ahhh, I didn't realize - as long as it is still correct, I think its totally fine to include those changes in this PR. I didnt approve just to be sure it was something we wanted to do

@inlined inlined merged commit a6f9851 into master Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants