-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
feat(ext/timers): add refTimer, unrefTimer API (alt) #12953
Conversation
25fe48c
to
b32e5d2
Compare
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.
Needs a rebase but #12862 was merged earlier today and this PR otherwise LGTM.
ext/timers/01_timers.js
Outdated
|
||
// Step 4 in "run steps after a timeout". | ||
MapPrototypeSet(activeTimers, id, cancelRid); | ||
timerInfo = { cancelRid, isRef: true }; |
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.
- timerInfo = { cancelRid, isRef: true };
+ timerInfo = { cancelRid, isRef: true, promiseId: -1 };
Keeps the object monomorphic.
ext/timers/01_timers.js
Outdated
@@ -255,6 +263,24 @@ | |||
clearTimeout(id); | |||
} | |||
|
|||
function refTimer(id = 0) { | |||
const timerInfo = MapPrototypeGet(activeTimers, id); | |||
if (timerInfo.isRef) { |
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.
if (timerInfo === undefined || timerInfo.isRef) {
(ditto on line 277)
The id = 0
default argument is kind of weird/footgun-y, I'd remove that.
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.
if (timerInfo === undefined || timerInfo.isRef) { (ditto on line 277)
Oops. Thanks!
The id = 0 default argument is kind of weird/footgun-y, I'd remove that.
I blindly followed the pattern used in clearTimeout/clearInterval, but I agree with your point
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.
LGTM too! Nice work @kt3k
Is there any timeline this might go from unstable to stable? I think this might be the only thing preventing me from being able to push jobs from Deno Deploy to BullMQ
|
@jcs224 How do you try to use Bull MQ in Deploy? Is it via esm.sh? Do you import |
I've been using the Node require polyfill, this works fine with the import { createRequire } from "https://deno.land/std@0.138.0/node/module.ts";
const require = createRequire(import.meta.url);
const { Queue } = require('bullmq')
const myQueue = new Queue('foo') However, when I try this: import { Queue } from 'https://esm.sh/bullmq'
const myQueue = new Queue('foo') I get a weird lodash error
Also, I've been doing this all with the CLI, not deploy. I know that Deploy only uses stable API so I didn't even bother to try. The node |
depends on #12862 (alternative to #12942)
This PR adds Deno.refTimer and Deno.unrefTimer APIs in the unstable namespace.
closes #6141