-
Notifications
You must be signed in to change notification settings - Fork 182
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
PDE-2564: core(feat): allow using await in inline function source (9.x backport) #390
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! My only thought here is if it being async changes our error-handling story at all. I don't think it does (and I see you've got a test), but I could see a situation where wherever this created function is called (the execute
command handler, I think?) we do a catch
but don't expect a rejected promise. So, if you haven't, that bears looking into.
The only other consideration isn't related to the content of this PR exactly, but about the backporting. Apps that have been running continuously (most public apps, which probably don't have their lambda functions pruned) could still be running on Node 8. It's possible that they don't work on later node versions, but we don't know it yet. We could be poking the bear by refreshing apps that have been humming along nicely. It's probably fine, but that was an area of concern that popped up as I was thinking through this.
Also, my guess is that we'll take godzilla apps from 9.x right to 11.x, so we probably don't need a 10.x release, FWIW.
Great job!
@xavdid thanks for the great feedback! I didn't think that much so I took a second look. I think we can safely assume the core always calls the created function asynchronously. The command handler named zapier-platform/packages/core/src/create-app.js Lines 43 to 45 in 717bbed
In zapier-platform/packages/core/src/middleware.js Lines 88 to 92 in 717bbed
This means even if Good call on the Node 8 concern! I looked into the database. No Visual Builder apps are running on Node.js 8. I think they all got upgraded in the previously 9.x releases. |
Sweet! Carry on 🚢 |
This is a 9.x backport merging into the
release-9.x
branch. I created this new branchrelease-9.x
from tagzapier-platform-cli@9.4.2
so we can release more9.x
versions from that branch. Once this PR is approved, I'll merge and cut a release 9.5.0.This PR addresses two Jira tickets:
async/await
support for Visual Builder (hackathon project)z.request
are logged #387 to 9.x to make sure callingz.request()
in the legacy script is loggedFor PDE-2564, this patch allows developers to use the
await
operator in Visual Builder's Code Mode. For example, Visual Builder may generate this code for auth testing:Now with the support for the
await
operator, developers can write this: