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

Fixes callback issue #60

Merged
merged 1 commit into from
Jul 28, 2020
Merged

Fixes callback issue #60

merged 1 commit into from
Jul 28, 2020

Conversation

omenocal
Copy link
Contributor

This PR addresses issue #58 . When using it with a Jovo application, the Lambda class of Jovo expects a callback, which is not being sent in line 59 of the Bespoken VA package. Also, the send function is expecting only one parameter, the payload, and not the error object that might be null but it goes before the payload. That's why I'm always getting a null response, because the payload object is getting the error null object returned by the Jovo framework.

@jkelvie
Copy link
Member

jkelvie commented Jul 28, 2020

Thanks for submitting this, Octavio!

@jkelvie jkelvie merged commit 43875d4 into bespoken:master Jul 28, 2020
@omenocal
Copy link
Contributor Author

@jkelvie my pleasure. I'm using bespoken virtual test packages for my unit tests in voice projects. Do you have an estimated date to release this and/or any other features to the NPM package ?

@dmarvp
Copy link
Contributor

dmarvp commented Jul 29, 2020

Hi @omenocal , we just published the latest version of the virtual-google-assistant with your change. Thanks!

@omenocal
Copy link
Contributor Author

@dmarvp just confirmed these changes work perfect. Thank you for the quick response!

@omenocal omenocal deleted the patch-1 branch July 29, 2020 15:23
@armonge
Copy link
Contributor

armonge commented Aug 14, 2020

@jkelvie @omenocal @dmarvp i've just had an issue after upgrading because of this change. According to the documentation the "handler" is expecting a "Google Cloud Function"

This change is essentially chaning the send signature to one that's not conformant to what a google cloud function is expecting

Eg:

exports.handler = (req, res) => {
  res.send('Hello, World');
};

https://cloud.google.com/functions/docs/functions-framework

@jkelvie
Copy link
Member

jkelvie commented Aug 14, 2020

Hi @armonge thanks for pointing this out. I should have looked at this more closely, because an error object as the first parameter to send is not familiar to me. My understanding is that Google modeled cloud functions on the Express.js API, which does not support such a parameter.

@omenocal - can you point me to the Jovo docs where the response.send is? I'd like to see that before proposing a solution.

@omenocal
Copy link
Contributor Author

Hi @jkelvie . The class that Jovo uses to return a response for Google projects is Lambda.ts, lines 74 and 83

https://github.com/jovotech/jovo-framework/blob/master/jovo-framework/src/hosts/Lambda.ts#L74

@jkelvie
Copy link
Member

jkelvie commented Aug 14, 2020

Thank you! It seems to me a flaw/limitation in what we are doing to be tightly coupled to Google Cloud Functions. This is probably something that we need to address.

As an immediate fix, it seems like we can serve both Cloud Functions and Alexa by leaving the send function alone, but passing a third parameter that looks like this:

callback(error, payload) {
   resolve(payload);
   return response;
}

send would revert to look like this:

send: (payload: any) => {
   resolve(payload);
   return response;
},

The final return statement would look like this:

return Promise.resolve(googleFunction(request, response, callback));

That should work for Jovo as well as someone using a Lambda with callbacks - and "regular" Lambdas as well. Does it sound like I'm on the right track?

@armonge
Copy link
Contributor

armonge commented Aug 15, 2020

I guess the issue comes mostly because if a misunderstanding, or even because of lack of consistency, virtual Alexa supports a lambda handler, this is not that. Should there maybe be a separate "lambda" handler?

@dmarvp
Copy link
Contributor

dmarvp commented Aug 24, 2020

Hi @armonge , @omenocal we released a new version recently (0.3.8). Could you please give it a try and let us know if these new changes are working for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants