ref(deno): Refactor deno integrations to use functional syntax #9929
Conversation
size-limit report 📦
|
mydea
force-pushed
the
fn/deno-integrations
branch
from
8b2b578 to
2d20567
Compare
| } | ||
|
|
||
| async function cronCalled(): Promise<void> { | ||
| if (getClient() !== client) { |
There was a problem hiding this comment.
@timfish checking this here to make sure this is only applies if this is the current client, otherwise it may send a monitor to a different client I guess.
There was a problem hiding this comment.
Part of me was thinking that you might still need to call fn() rather than return but now I'm not sure.
I'm struggling to get my head around how it'll all behave if wrapped multiple times 🤔
There was a problem hiding this comment.
This patch should be done once, not per client, because it proxies the global Deno.cron function, but we still need the getClient() !== client so that monitors from clients with the integration get sent.
Can we check if client has integration installed and check accordingly?
We might have to audit all of our integrations for this behaviour now that I think about it.
There was a problem hiding this comment.
I've actually rewritten most of them this way, but you're right, it makes more sense this way here too - will follow the same as I did in other PRs already!
mydea
force-pushed
the
fn/deno-integrations
branch
from
2d20567 to
b649c94
Compare
mydea
force-pushed
the
fn/deno-integrations
branch
from
b649c94 to
ad1ec1e
Compare
| const denoCronIntegration = (() => { | ||
| return { | ||
| name: INTEGRATION_NAME, | ||
| setupOnce() { |
There was a problem hiding this comment.
@AbhiPrasad / @timfish , ok, hopefully final refactor here - now this registers once in setupOnce, and registers the client in setup, which we check for in here!
|
|
||
| async function cronCalled(): Promise<void> { | ||
| if (SETUP_CLIENTS.includes(getClient() as Client)) { | ||
| return; |
There was a problem hiding this comment.
Don't we still want to call fn here?
| import { parseScheduleToString } from './deno-cron-format'; | ||
|
|
||
| type CronOptions = { backoffSchedule?: number[]; signal?: AbortSignal }; | ||
| type CronFn = () => void | Promise<void>; | ||
| // Parameters<typeof Deno.cron> doesn't work well with the overloads 🤔 | ||
| type CronParams = [string, string | Deno.CronSchedule, CronFn | CronOptions, CronFn | CronOptions | undefined]; | ||
|
|
||
| const INTEGRATION_NAME = 'DenoCron'; | ||
|
|
||
| const SETUP_CLIENTS: Client[] = []; |
There was a problem hiding this comment.
This means that clients dont get GC'd - I wonder if we should track with a WeakMap instead?
Refactors deno integrations to functional syntax.