oven-sh / bun

Incredibly fast JavaScript runtime, bundler, test runner, and package manager – all in one

https://bun.sh

Other

74.23k stars 2.77k forks source link

Implement `promiseHooks` from `node:v8` #6136

Open bkniffler opened 1 year ago

bkniffler commented 1 year ago

What is the problem this feature would solve?

When trying to use temporal sdk (https://github.com/temporalio/sdk-typescript), getting error

node:v8 createHook is not yet implemented in Bun

Its used right here: https://github.com/temporalio/sdk-typescript/blob/3ce098be042e86d26ea5b433508a480503cb09b2/packages/worker/src/workflow/vm-shared.ts#L198

What is the feature you are proposing to solve the problem?

Implement createHook.

What alternatives have you considered?

Ask temporal to not use createHook, but probably out of reach since its needed to isolate the VM scope.

Electroid commented 1 year ago

We'd have to think about if this is worth implementing, since it could have a performance impact. (it tracks all promises) However, even if it is feasible, it's not a high priority.

alirezabonab commented 11 months ago

@Electroid Is there any update on this?

danieldunderfelt commented 9 months ago

Sadly the Temporal Typescript SDK does not work without createHook 😩

I'd love to have it at least as an opt-in if the performance impact is big.

Ping @lorensr

loicnestler commented 7 months ago

What's the status on this? Is an implementation of createHook planned?

bergundy commented 6 months ago

I'm the author of the code you linked in the Temporal TypeScript SDK. Promise hooks are optional, they allow us to capture the stack trace of all running functions in a workflow. I'm not sure if there's more of a gap for running our SDK on top of Bun but we can look into it. Apart from promise hooks, we use Node's vm and worker_threads modules, AsyncLocalStorage, and rely heavily on Rust Neon bindings.

paradoxloop commented 1 month ago

@bergundy if Promise Hooks are optional is there a way to disable them so that we can use Bun. There is a linked issue on the Temporal project.

axe-me commented 1 month ago

Can bun simply keep silent about this instead of throw an error, if there is no plan to implement this in short time. Just like what deno did in this PR: https://github.com/denoland/deno/pull/25979/files

maybe just use warnNotImplementedOnce in here: https://github.com/oven-sh/bun/blob/25fcbed8d187742faedfe2ff7ce413c059a3c774/src/js/node/v8.ts#L101-L117

or just simply remove the promiseHooks object

bergundy commented 1 month ago

14485 seems like the way to go. If it ends up not getting merged, we may be able to do something on the Temporal SDK side, it should be easy to edit the SDK files post install to remove the use of promise hooks and confirm.

I’m subscribed to this issue, wondering if this is the last hurdle for running the Temporal SDK on bun.