feat: Webhook max lifetime #1815
Conversation
src/subscriptions.ts
Outdated
| import { parseVariable } from './parse.js'; | ||
| import { logger } from './logger.js'; | ||
| import { ObjMap } from 'graphql/jsutils/ObjMap.js'; | ||
| import { pipeline, Writable } from 'readable-stream'; |
There was a problem hiding this comment.
In order to keep the implementation environment agnostic, is it possible to use WritableStream from @whatwg-node/fetch that we use across the library instead.
There was a problem hiding this comment.
Thats what I kinda was going for with readable-stream. They use the native stream implementation if available and fall back to userland implementation if they aren't. But I'll try your suggestion with the fetch package, but I think they do not provide a pipeline operator which automatically handles backpressure etc.?
There was a problem hiding this comment.
Yeah I kinda ran into some issues so I think it would be the most straight forward thing to rely on the readable-stream package if this is not an issue from your side!
There was a problem hiding this comment.
As far as I see on npm, it is a mirror of the builtin streams pacakge of Node. And if we have this package in SOFA, we will no longer support non-Node environments. I'd prefer to have an agnostic solution with the standard ReadableStream, WriteableStream etc.
I think for maximum lifetime, we can rely on setTimeout and call of stop in case of timeout instead of refactoring whole logic with Node-specific API.
There was a problem hiding this comment.
For the stop part, this is what currently is happening, I use setTimeout to schedule a run for termination later on.
Stream wise, the support for the api is pretty good: https://developer.mozilla.org/en-US/docs/Web/API/WritableStream#browser_compatibility
Node, Bun and Deno all support the api natively. For browser and backwards consistency, the readable-stream package does the heavy lifting.
Would you prefer the implementation to use the native streams API directly?
There was a problem hiding this comment.
But unfortunately this has the same problem. stream package is not available on non-Node environment. If we merge this as-is. We will no longer be able to support non-Node environments like we do today.
There are other non-Node evnironments that don't support Node-specific builtin packages yet such as CF Workers(it has some kind of support with custom flags which add some overhead).
There was a problem hiding this comment.
If I'm not mistaken, at least CF workers follows the WinterTC API standard, which does support WritableStreams: https://min-common-api.proposal.wintertc.org/#api-index / https://github.com/cloudflare/workerd/blob/46fd550189f0016933ae6babb4397a8576424f26/src/workerd/api/node/tests/streams-test.js#L29
But nontheless I see your point. Would a runtime check for the api availability + a dynamic import of the api in case it is available erase your concerns?
There was a problem hiding this comment.
WriteableStreams are not Writable of Node.js. They are different APIs.
What if api is not available for the environment then we will disable the webhooks feature for that environment, right?
There was a problem hiding this comment.
I see. I fully removed the use of streams.
There was a problem hiding this comment.
Hey, did you have the chance to take a look yet? Would you like me to change or improve something?
2 participants
Following up to this discussion here is my suggestion of a max lifetime implementation for SOFA webhooks.
Let me know what you think, I'm looking forward to your feedback!