-
Notifications
You must be signed in to change notification settings - Fork 629
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(node): add Timeout class #1699
Conversation
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.
I'm against merging this before tomorrow's meeting
@bartlomieju Can you outline your objections? |
@bnoordhuis importing |
Okay, so we discussed this PR during the design meeting. The consensus was that we shouldn't mess with global types - we should keep Web compatible types and we shouldn't override We searched using SourceGraph for packages using |
a968f0b
to
e6960e9
Compare
I reverted exposing global part of this PR for now.
I feel that might be very difficult because they have small motivation to accept such PRs (in case of mocha, they probably even don't have time to start reviewing it. They seem very inactive.), but let's see how it works for a while. |
Seconded. I think you're being way too optimistic on the probability of such PRs getting accepted and merged/released in a timely fashion. To maintainers of those packages it's all downside and little or no upside. We're aiming for end of January for v0. Being blocked on third parties is not an option. |
Merged main. This PR now doesn't include |
node/_http_client.js
Outdated
// Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. | ||
// Copyright Joyent and Node contributors. All rights reserved. MIT license. |
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.
I suspect this file was accidentally resurrected during a rebase?
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.
Oh, thanks! This was a mistake during resolving merge conflicts!
Merging this as it doesn't include the controversial part of timers compatibility issue. I'm going to try to implement the idea of providing node-compatible timers via commonJS wrapper function |
closes #1622