[kreddlear]

Summary

This PR removes the dependency on the Bluebird package and modernizes async handling across the codebase by switching to native Promises and async/await.

Changes

• Removed usage of the Bluebird package in favor of native Promises - the Bluebird library is no longer actively maintained nor recommended, and its presence is causing issues for the Integrations teams, especially with Typescript
  • This also means that Promise is no longer globally patched with Bluebird Promises - instead Promise refers to a native Promise as would normally be expected.
• Replaced Bluebird error contextifying with a whatHappened array of strings stored in the _zapier object, to ensure error context is still bubbled up as expected
• Added enrichErrorMessages method to middleware to enable above error context
• Updated Lambda handler to use async/await instead of callbacks
• Refactored middleware functions to use async/await for improved readability and maintainability

Motivation

Simplifies the async flow, reduces external dependencies, and aligns the codebase with modern JavaScript practices.

[kreddlear]

Removing this since we're not using callbacks anymore, so it likely will not apply. Need to remove the corresponding z/z code and Gargoyle switch as well - context here, AWS docs here

[eliangcs]

I think it's good to remove callbackWaitsForEmptyEventLoop. 👍

Side note: It's not entirely clear how AWS Lambda behaves when we have unawaited promises with async/await. I did some experimenting and found the following:

• If an unawaited promise is rejected, Lambda might return an error—but not always. Sometimes the function completes successfully. When it does error and crash, Lambda appears to re-initialize the function on the next invocation.
• If an unawaited promise resolves successfully, Lambda always returns successfully without waiting for the promise to finish.

With callbacks, the behavior is more predictable:

The function continues to execute until the event loop is empty or the function times out, unless context.callbackWaitsForEmptyEventLoop is set to false.

So it's as if context.callbackWaitsForEmptyEventLoop is now always false when we refactor code from callbacks to async/await. I assume that's fine because we don't rely on callbacks to finish anything in background. The only way to make sure this behavioral difference doesn't break existing integrations is canary testing, I think.

[kreddlear]

So it's as if context.callbackWaitsForEmptyEventLoop is now always false when we refactor code from callbacks to async/await. I assume that's fine because we don't rely on callbacks to finish anything in background. The only way to make sure this behavioral difference doesn't break existing integrations is canary testing, I think.

Yes, this is my impression as well. I think it will be fine, but it's hard to know exactly how integrations will behave.

Are there any particular integrations/integration types you'd recommend to canary to make sure that this doesn't break certain functionality? I'll be canary testing integrations relying on the legacy scripting runner, and if you have ideas for other integration functionality to focus on, I'd be happy to test those cases as well.

[eliangcs]

I think we should canary test on a few popular integrations, starting with the ones that have few users, of course. But I agree integrations relying on legacy-scripting-runner are good candidates too.

[kreddlear]

I think we want to call logger.end() here after everything else in the handler - however, I'm not super familiar with the logger setup or the /input endpoint. Would appreciate feedback if this doesn't look right.

[eliangcs]

I think you're right. Good catch!

[kreddlear]

Since we are deleting the core/src/tools/promise.js file above, which mostly powers this test file, do we want to delete the test file as well?

The only remaining failing tests are these 3 contextual promise tests. I have not added the ability to "contextify" via a function in this PR, rather context is just handled by pushing up strings to the whatHappened array in _zapier (credit to ChangHung for this idea from the spike PR).

I wasn't able to find a strong reason to have this "contextify" functionality but am open to adding it if it's necessary.

[eliangcs]

I don't think there's a reason to keep the "contextify" functionality either. So yeah, let's remove the test file.

[kreddlear]

@eliangcs 👋 Hello, thanks again for all your help so far! This MR is almost ready for review, I'll spend time tomorrow canarying it using a test platform version to make sure it is fully ready. In the meantime though, I'm wondering if you could take a look at this question above and the failing tests here?

My basic question is if you think it's worth refactoring to keep the logic to pass a function in to "contextify" results, or if we should just remove the test file for ZapierPromise along with the promise.js logic itself. I'd appreciate any thoughts, thank you in advance! And of course if there are any other red flags with the PR that stand out right away, please feel free to bring them to my attention as well, it should be mostly done.

[kreddlear]

I added this promise logic here since the legacy scripting beforeRequest logic can now return a promise.

[kreddlear]

This should have been removed in the previous ESM PR.

[eliangcs]

Left a few comments. I don't see big issues.

I'd like to test it further using zapier invoke, but it didn't work because the previous changes with ESM didn't work well with the zapier invoke implementation in the main branch. I'll solve merge conflict between this and main (work-in-progress PR) and continue my test.

[eliangcs]

I think it's good to remove callbackWaitsForEmptyEventLoop. 👍

Side note: It's not entirely clear how AWS Lambda behaves when we have unawaited promises with async/await. I did some experimenting and found the following:

• If an unawaited promise is rejected, Lambda might return an error—but not always. Sometimes the function completes successfully. When it does error and crash, Lambda appears to re-initialize the function on the next invocation.
• If an unawaited promise resolves successfully, Lambda always returns successfully without waiting for the promise to finish.

With callbacks, the behavior is more predictable:

The function continues to execute until the event loop is empty or the function times out, unless context.callbackWaitsForEmptyEventLoop is set to false.

So it's as if context.callbackWaitsForEmptyEventLoop is now always false when we refactor code from callbacks to async/await. I assume that's fine because we don't rely on callbacks to finish anything in background. The only way to make sure this behavioral difference doesn't break existing integrations is canary testing, I think.

[eliangcs]

Is it possible to add test case(s) to ensure that this is true? That, if a callback throws an error somewhere, this block of code will catch it.

[eliangcs]

Update: I verified it locally by writing a trigger perform method like this:

const perform = async (z, bundle) => {
  setTimeout(() => {
    throw new Error('Random error!');
  }, 1);
  const response = await z.request({
    url: 'https://auth-json-server.zapier-staging.com/recipes',
  });
  return response.data;
};

When running the trigger with zapier invoke trigger recipe --debug, I can see the "Uncaught error: ..." error message.

$ zapier-dev invoke trigger recipe --debug
... (omitted)
  zapier:invoke Uncaught error: Error: Random error!
  zapier:invoke Error: Random error!
  zapier:invoke     at Timeout._onTimeout (/Users/eliang/Projects/zapps/oauth/triggers/recipe.js:3:11)
  zapier:invoke     at listOnTimeout (node:internal/timers:581:17)
  zapier:invoke     at process.processTimers (node:internal/timers:519:7) +18ms
  zapier:invoke {
  zapier:invoke   err: Error: Random error!
  zapier:invoke       at Timeout._onTimeout (/Users/eliang/Projects/zapps/oauth/triggers/recipe.js:3:11)
  zapier:invoke       at listOnTimeout (node:internal/timers:581:17)
  zapier:invoke       at process.processTimers (node:internal/timers:519:7) {
  zapier:invoke     domainThrown: true
  zapier:invoke   },
  zapier:invoke   log_type: 'error',
  zapier:invoke   request_headers: undefined,
  zapier:invoke   response_headers: undefined
  zapier:invoke } +0ms
✖ Invoking triggers.recipe.operation.perform
  oclif:zapier:invoke Error: Random error!
  oclif:zapier:invoke     at Timeout._onTimeout (/Users/eliang/Projects/zapps/oauth/triggers/recipe.js:3:11)
  oclif:zapier:invoke     at listOnTimeout (node:internal/timers:581:17)
  oclif:zapier:invoke     at process.processTimers (node:internal/timers:519:7) +102ms
 ›   Error: Random error!
 ›
 ›   Console logs:
 ›

That means we can expect the uncaught error to be logged.

[eliangcs]

I think you're right. Good catch!

[eliangcs]

I found some issues while working on #997.

1. bluebird is still in packages/core/package.json. We should remove it.

2. It's possible for a developer to use zapier-platform-cli v17 on zapier-platform-core v16. I tried to run zapier validate and zapier describe on an example integration that uses core v16, and saw an error saying Cannot set properties of undefined (setting 'callbackWaitsForEmptyEventLoop'):

$ zapier-dev validate --debug
  oclif:zapier:validate argv is [] +0ms
  oclif:zapier:validate args are {} +1ms
  oclif:zapier:validate flags are { debug: true, format: 'table' } +0ms
  oclif:zapier:validate ------------ +0ms
Validating project locally
  oclif:zapier:validate TypeError: Cannot set properties of undefined (setting 'callbackWaitsForEmptyEventLoop')
  oclif:zapier:validate     at handler (/Users/eliang/Projects/zapps/todoist/node_modules/zapier-platform-core/src/tools/create-lambda-handler.js:195:44)
  oclif:zapier:validate     at /Users/eliang/Projects/zapier-platform/packages/cli/src/utils/local.js:343:18
  oclif:zapier:validate     at localAppCommand (/Users/eliang/Projects/zapier-platform/packages/cli/src/utils/local.js:358:26)
  oclif:zapier:validate     at async ValidateCommand.perform (/Users/eliang/Projects/zapier-platform/packages/cli/src/oclif/commands/validate.js:14:20)
  oclif:zapier:validate     at async Promise.all (index 1)
  oclif:zapier:validate     at async ValidateCommand._run (/Users/eliang/Projects/zapier-platform/node_modules/@oclif/core/lib/command.js:312:22)
  oclif:zapier:validate     at async Config.runCommand (/Users/eliang/Projects/zapier-platform/node_modules/@oclif/core/lib/config/config.js:435:25)
  oclif:zapier:validate     at async run (/Users/eliang/Projects/zapier-platform/node_modules/@oclif/core/lib/main.js:95:16)
  oclif:zapier:validate     at async /Users/eliang/Projects/zapier-platform/packages/cli/src/bin/run:5:3 +67ms
 ›   Error: Cannot set properties of undefined (setting 'callbackWaitsForEmptyEventLoop')

It's because the way localAppCommand calls the Lambda handler is now different. On v16 we call it with a callback, whereas on v17 we call it with await.

For a good developer experience, zapier-platform-cli v17 should work with zapier-platform-core v16. But if that's not possible, we should at least print a clear error message telling the developer to upgrade to zapier-platform-core v17.

[eliangcs]

Update: 2️⃣ mentioned above has been fixed in 1855605, so don't worry about it in this PR.

[eliangcs]

Update: bluebird removed in bcd28a6.

[eliangcs]

This looks good to me now. There's a merge conflict with schema.md. Once you resolve it, feel free to merge. Thanks for the great work!

[kreddlear]

Thanks so much @eliangcs ! I resolved the conflict and added some test cases for the Uncaught error vs Unhandled error logging per your comment here - I had also confirmed the behavior locally, but it seems good to have that confirmed in tests as well. I'll merge this into v17 and continue canary testing 👍

[eliangcs]

Thanks for adding the test!