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

std/node: Node compatible setTimeout, setInterval #1622

Closed
Tracked by #12886
kt3k opened this issue Nov 22, 2021 · 6 comments · Fixed by #1699
Closed
Tracked by #12886

std/node: Node compatible setTimeout, setInterval #1622

kt3k opened this issue Nov 22, 2021 · 6 comments · Fixed by #1699
Assignees
Labels
node Related to node compatibility layer

Comments

@kt3k
Copy link
Member

kt3k commented Nov 22, 2021

setTimeout in Node.js returns Timeout object

ref(Node.js):

We have Timeout class implemented in std/node, but doesn't seem implementing setTimeout correctly

@bartlomieju
Copy link
Member

I'm not sure how we can do that, it's the same problem as in #1455.

I'm very much against overriding globals depending if we're in compat mode or Node, ie. we should stick to Deno's globals anyway. I discussed it with @dsherret, but there are no ideas how to reconcile the two, the globals are just incompatible.

@bnoordhuis any ideas?

@bartlomieju bartlomieju added the node Related to node compatibility layer label Nov 22, 2021
@kt3k
Copy link
Member Author

kt3k commented Nov 22, 2021

But this difference actually prevents running mocha's integration test in compat mode as they use setInterval().unref() in their testing utility. ref: https://github.com/mochajs/mocha/blob/3b4cc05f3fa0502d5d714a8f62ac8785267aa695/lib/nodejs/parallel-buffered-runner.js#L307-L310

You can reproduce the above issue with the following commands.

git clone https://github.com/mochajs/mocha.git
cd mocha
npm install
# run integration test
deno run --unstable --compat -A --location https://deno.land bin/mocha --parallel --timeout 10000 --slow 3750 "test/integration/**/*.spec.js"

How about overriding these in compat mode, but discouraging the use of these non-standard features somehow, for example, by warning in the linter

@lucacasonato
Copy link
Member

lucacasonato commented Nov 22, 2021

I'd say it is probably safe to override them in compat mode, if Symbo.toPrimitive is implemented on the timers object. Very ugly though (and very painful). See responses to https://twitter.com/lcasdev/status/1462766579196862470

@Jack-Works
Copy link

I'd say it is probably safe to override them in compat mode, if Symbo.toPrimitive is implemented on the timers object. Very ugly though (and very painful). See responses to https://twitter.com/lcasdev/status/1462766579196862470

Node also has Symbol.toPrimitive on its timer class.

@kt3k kt3k self-assigned this Nov 24, 2021
@kt3k kt3k changed the title std/node: Node compatible setTimout, setInterval std/node: Node compatible setTimeout, setInterval Nov 24, 2021
@kt3k
Copy link
Member Author

kt3k commented Nov 24, 2021

The issue denoland/deno#12745 might be related if we want to implement setInterval().unref()

@bnoordhuis
Copy link
Contributor

Channeling @lucacasonato: idea is to monkey-patch setTimeout/setInterval globals and return objects instead of numbers in --compat mode.

(My recollection was to return objects always but I might be misremembering and I don't have a strong preference anyway.)

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

Successfully merging a pull request may close this issue.

5 participants