Skip to content
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

refactor: use setTimeout from timers module #43

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bartlomieju
Copy link

A small change that uses setTimeout from timers module instead of relying on a global.

I'm trying to get tap running in Deno's Node.js compat mode and return types for setTimeout and setInterval are fundamentally incompatible - Deno follow's Web APIs and returns a number, while Node.js returns and object that allows to ref/unref the timer. AFAIK there's no other way to unref a timer in Node.js and this is one of the incompatibilities we haven't yet figured out how to handle nicely. Using setTimeout from timers module allows us to polyfill that API properly without changing return types on built-in globals.

I'm not really sure how we could test this change, suggestions are appreciated.

@isaacs
Copy link
Member

isaacs commented Mar 7, 2022

This is a good change, I'm happy to support Deno. I think you'll have to pull in clearTimeout from the timers module as well, though.

The easiest way to test this would be to mock the timers module. Something like this:

// somewhere in test/base.js
const { setTimeout, clearTimeout } = require('timers')
const mockTimers = {
  timeoutsSet: [],
  timeoutsCleared: [],
  setTimeout: (fn, time) => {
    const handle = setTimeout(fn, time)
    timeoutsSet.push([handle, fn, time])
    return handle
  },
  clearTimeout: (handle) => {
    const ret = clearTimeout(handle)
    timeoutsCleared.push(handle)
    return ret
  }
}

const Base = t.mock('../lib/base.js', { timers: mockTimers })
const b = new Base({name: 'timer mock test'})
b.setTimeout(1000)
b.setTimeout(0)
// now verify that mockTimers.timeoutsSet and mockTimers.timeoutsCleared
// contain some entries, that they're what you expect, etc.

@bartlomieju
Copy link
Author

Thanks for pointers @isaacs, I added a test. Let me know if there's anything else I should do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants