allow for nested functions from firebase #191
Conversation
|
Has some conflicts that need to be resolved. |
src/cli/controller.js
Outdated
| return this.client.generateUploadUrl(this.config) | ||
| .then(([body]) => { | ||
| cloudfunction.sourceUploadUrl = body.uploadUrl; | ||
| CloudFunction.addLocaldir(cloudfunction, opts.source); | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| // Parse the user's code to find the names of the exported functions | ||
| if (opts.firebase) { | ||
| resolve(name.replace(/\-/g, '.')); |
There was a problem hiding this comment.
I see you're resolving the promise right here, do you also want to add a return statement to avoid the exec call on line 120?
src/supervisor/worker.js
Outdated
| @@ -73,7 +74,12 @@ function main () { | |||
|
|
|||
| // Require the target module to load the function for invocation | |||
| const functionModule = require(localdir); | |||
| const handler = functionModule[cloudfunction.entryPoint || name]; | |||
| const bracketedName = name.split('-'); | |||
| const nestedFunctionModule = _.reduce(bracketedName, function(funcMod, item) { | |||
There was a problem hiding this comment.
Change to:
const nestedFunctionModule = bracketedName.reduce((funcMod, item) => funcMod[item], functionModule);
|
Are you ready for this to be merged? |
src/supervisor/worker.js
Outdated
| @@ -73,7 +74,7 @@ function main () { | |||
|
|
|||
| // Require the target module to load the function for invocation | |||
| const functionModule = require(localdir); | |||
| const handler = functionModule[cloudfunction.entryPoint || name]; | |||
| const handler = _.get(functionModule, cloudfunction.entryPoint, name); | |||
There was a problem hiding this comment.
should change this to:
const handler = _.get(functionModule, cloudfunction.entryPoint, functionModule[name]);
or
const handler = _.get(functionModule, cloudfunction.entryPoint) || functionModule[name];
src/cli/controller.js
Outdated
| @@ -114,6 +114,10 @@ class Controller { | |||
|
|
|||
| return new Promise((resolve, reject) => { | |||
| // Parse the user's code to find the names of the exported functions | |||
| if (opts.firebase) { | |||
| // If call is coming from Firebase, assume functions have already been parsed | |||
| return resolve(name.replace(/\-/g, '.')); | |||
There was a problem hiding this comment.
I don't think you have to do the replace, since the Firebase CLI would have already done the dash to dot substitution during the trigger parsing step. Can replace this line with:
return resolve(name)
But you should test to make sure.
There was a problem hiding this comment.
The CLI does not do the dot substitution. We could also go with opts.entryPoint here, instead of replacing the dashes with dots in the name.
There was a problem hiding this comment.
Actually, I got confused. What I meant is that it is fine to leave the name with dashes in them. You do not need to put the dots back. When the Firebase CLI deploys functions to the real GCF API, the name of each function stays the "dashed" version, since the API doesn't actually allow dots in the name of functions. So you should still replace this line with: return resolve(name)
src/supervisor/worker.js
Outdated
| @@ -73,7 +74,7 @@ function main () { | |||
|
|
|||
| // Require the target module to load the function for invocation | |||
| const functionModule = require(localdir); | |||
| const handler = functionModule[cloudfunction.entryPoint || name]; | |||
| const handler = _.get(functionModule, cloudfunction.entryPoint, functionModule[name]); | |||
There was a problem hiding this comment.
Actually, this would be cleaner as:
const handler = _.get(functionModule, cloudfunction.entryPoint || name);
|
@laurenzlong approval? |
|
Yep. |
|
Is this ready on @latest ? |
|
Not yet
…On Fri, Mar 2, 2018, 3:10 PM Victor Nascimento ***@***.***> wrote:
Is this ready on @latest <https://github.com/latest> ?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#191 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABKERqSKSLY5I5Zbp-pEcb67PpzshcLzks5tadFogaJpZM4SQQpJ>
.
|
Companion to firebase/firebase-tools#677.
Fixes firebase/firebase-tools#481