feat: add AbortSignal.timeout(number) #60628

zone117x · 2022-06-01T22:07:32Z

Add AbortSignal.timeout(number) definition to @types/node, available since Nodejs v17.3.0, v16.14.0.

See https://nodejs.org/api/globals.html#static-method-abortsignaltimeoutdelay for more details.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change.
Follow the advice from the readme.
Avoid common mistakes.
Run npm test <package to test>.

Select one of these and delete the others:

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: https://nodejs.org/api/globals.html#static-method-abortsignaltimeoutdelay
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.

typescript-bot · 2022-06-01T22:08:09Z

@zone117x Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

node — on npm, on unpkg
- owner-approval: @mcollina

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PR in the Playground.

Status

✅ No merge conflicts
❌ Continuous integration tests have failed
🕐 Most recent commit is approved by a DT maintainer

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Inactive

This PR has been inactive for 31 days — it is considered abandoned, and therefore closed!

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 60628,
  "author": "zone117x",
  "headCommitOid": "1ee7f133f45f1375cd370c5b63a7b61dab3a0180",
  "mergeBaseOid": "e8b369c1b9d01c667aa871bc714dd23561a21cea",
  "lastPushDate": "2022-06-02T12:59:12.000Z",
  "lastActivityDate": "2022-06-10T01:47:19.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/globals.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v14/globals.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/globals.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/test/globals.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "Microsoft",
        "DefinitelyTyped",
        "jkomyno",
        "alvis",
        "r3nya",
        "btoueg",
        "smac89",
        "touffy",
        "DeividasBakanas",
        "eyqs",
        "Hannes-Magnusson-CK",
        "hoo29",
        "kjin",
        "ajafff",
        "islishude",
        "mwiktorczyk",
        "mohsen1",
        "n-e",
        "galkin",
        "parambirs",
        "eps1lon",
        "SimonSchick",
        "ThomasdenH",
        "WilcoBakker",
        "wwwy3y3",
        "samuela",
        "kuehlein",
        "bhongy",
        "chyzwar",
        "trivikr",
        "yoursunny",
        "qwelias",
        "ExE-Boss",
        "peterblazejewicz",
        "addaleax",
        "victorperin",
        "ZYSzys",
        "NodeJS",
        "LinusU",
        "wafuwafu13",
        "mcollina"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "mcollina",
      "date": "2022-06-05T11:49:11.000Z",
      "isMaintainer": false
    },
    {
      "type": "stale",
      "reviewer": "eps1lon",
      "date": "2022-06-01T23:21:14.000Z",
      "abbrOid": "c77989d"
    }
  ],
  "mainBotCommentID": 1144190626,
  "ciResult": "fail",
  "ciUrl": "https://github.com/DefinitelyTyped/DefinitelyTyped/commit/1ee7f133f45f1375cd370c5b63a7b61dab3a0180/checks?check_suite_id=6764929039"
}

typescript-bot · 2022-06-01T22:08:11Z

🔔 @microsoft @DefinitelyTyped @jkomyno @alvis @r3nya @btoueg @smac89 @Touffy @DeividasBakanas @eyqs @Hannes-Magnusson-CK @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @galkin @parambirs @eps1lon @SimonSchick @ThomasdenH @WilcoBakker @wwwy3y3 @samuela @kuehlein @bhongy @chyzwar @trivikr @yoursunny @qwelias @ExE-Boss @peterblazejewicz @addaleax @victorperin @ZYSzys @nodejs @LinusU @wafuwafu13 @mcollina — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

typescript-bot · 2022-06-01T23:19:12Z

@zone117x The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

types/node/v14/globals.d.ts

…lib/lib.dom.d.ts`

typescript-bot · 2022-06-02T12:26:05Z

@zone117x The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

zone117x · 2022-06-02T12:57:37Z

8000

@peterblazejewicz I'm struggling with getting this to work. It looks like the problem is that AbortSignal is defined in typescript/lib/lib.dom.d.ts as well as in @types/node, so if I add a property I'm getting the error:

Subsequent variable declarations must have the same type.  Variable 'AbortSignal' must be of type '{ new (): AbortSignal; prototype: AbortSignal; }', but here has type 'AbortSignalConstructor'.

lib.dom.d.ts(1923, 13): 'AbortSignal' was also declared here.

Is it possible to make this change without also changing typescript/lib/lib.dom.d.ts? Is there a way to decouple the two types? I'm not even sure why lib.dom is being included in this context.

typescript-bot · 2022-06-02T13:39:25Z

@zone117x The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

peterblazejewicz · 2022-06-02T20:59:20Z

I assume this is because some setups use, say:

/// <reference types="node" />

(a global reference)
and within tsconfig:

lib: [ "dom" ]

this results in both being used to resolve the same symbol.
Old problem without any clear solution as of yet. Simon, which done a ton of work for @types/node recently, created this TS issue:
microsoft/TypeScript#43972

yoursunny · 2022-06-02T21:24:49Z

How about making @types/web a peer dependency of @types/node, then importing types from that library?
This ensures consistency between typings, for projects that target both Node and browsers.

mcollina

lgtm

zone117x · 2022-06-06T15:37:47Z

@peterblazejewicz

Old problem without any clear solution as of yet. Simon, which done a ton of work for @types/node recently, created this TS issue:
microsoft/TypeScript#43972

Ah, looks like the issue keeps getting pushed as well. I've been looking at how this is handled in similar scenarios, for example with the URL & URLSearchParams globals:

DefinitelyTyped/types/node/url.d.ts

Lines 860 to 887 in ee088b6

    
               global { 
        
                   interface URLSearchParams extends _URLSearchParams {} 
        
                   interface URL extends _URL {} 
        
                   interface Global { 
        
                       URL: typeof _URL; 
        
                       URLSearchParams: typeof _URLSearchParams; 
        
                   } 
        
                   /** 
        
                    * `URL` class is a global reference for `require('url').URL` 
        
                    * https://nodejs.org/api/url.html#the-w
8000
hatwg-url-api 
        
                    * @since v10.0.0 
        
                    */ 
        
                   var URL: 
        
                       // For compatibility with "dom" and "webworker" URL declarations 
        
                       typeof globalThis extends { onmessage: any, URL: infer URL } 
        
                           ? URL 
        
                           : typeof _URL; 
        
                   /** 
        
                    * `URLSearchParams` class is a global reference for `require('url').URLSearchParams` 
        
                    * https://nodejs.org/api/url.html#class-urlsearchparams 
        
                    * @since v10.0.0 
        
                    */ 
        
                   var URLSearchParams: 
        
                       // For compatibility with "dom" and "webworker" URLSearchParams declarations 
        
                       typeof globalThis extends { onmessage: any, URLSearchParams: infer URLSearchParams } 
        
                           ? URLSearchParams 
        
                           : typeof _URLSearchParams; 
        
               }

Does an approach like this make sense for AbortController & AbortSignal? One potential issue is that since the DOM definition doesn't include AbortSignal.timeout(number), this workaround would make it so that the projects that reference the dom lib will use the dom AbortSignal definition rather than the @types/node one (if I am understanding this workaround).

@yoursunny

How about making @types/web a peer dependency of @types/node, then importing types from that library?
This ensures consistency between typings, for projects that target both Node and browsers.

Interesting, is there precedent for doing something like this, especially in this package?

zone117x · 2022-06-06T15:40:09Z

@antongolub it looks like you have experience with this situation (like in #58619) -- any suggestions here?

yoursunny · 2022-06-06T15:44:56Z

How about making @types/web a peer dependency of @types/node, then importing types from that library?
This ensures consistency between typings, for projects that target both Node and browsers.

Interesting, is there precedent for doing something like this, especially in this package?

No precedent, but this could be a solution.

A drawback is that the typing would not be able to capture the difference between web standards and Node implementation.
If a certain method or property is available in browsers but unavailable in Node, the typing would still indicate that it is available.
But then, this situation already exists when developer needs to do runtime feature testing to detect browser support for methods and properties that are not universally available.
They just have to do the same sort of runtime feature testing in Node instead of expecting the typing to accurately match the running Node version.

peterblazejewicz · 2022-06-06T19:07:55Z

@zone117x I'm not using Node on daily basis (.NET stack here), I do not have opinion on this one, as this was always problematic and I do not have a great experience on seeing in wider context a subtle change and it's possible ramification(s). The issue you've linked #58019 was the one approved by MS team member, e.g. see this comment on what I mean:
#58619 (comment)

zone117x · 2022-06-09T13:39:53Z

@rbuckton do you have any recommendations for a workaround here?

rbuckton · 2022-06-10T01:47:19Z

Unfortunately, I think the only solution is to update lib.dom in TypeScript to also have AbortSignalConstructor. Starting in TS 4.5 it's now possible to override lib.dom via a package dependency, but I don't think that would help in this case.

typescript-bot · 2022-07-03T02:57:16Z

@zone117x I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on Jul 10th (in a week) if the issues aren't addressed.

typescript-bot · 2022-07-11T02:56:36Z

@zone117x To keep things tidy, we have to close PRs that aren't mergeable and don't have activity in the last month. No worries, though — please open a new PR if you'd like to continue with this change. Thank you!

zone117x requested a review from peterblazejewicz as a code owner June 1, 2022 22:07

typescript-bot added the Critical package label Jun 1, 2022

feat: add AbortSignal.timeout(number)

c77989d

zone117x force-pushed the fix/node-abortsignal-timeout branch from ee4dc2b to c77989d Compare June 1, 2022 22:30

typescript-bot added the The CI failed When GH Actions fails label Jun 1, 2022

eps1lon reviewed Jun 1, 2022

View reviewed changes

types/node/v14/globals.d.ts Outdated Show resolved Hide resolved

fix type AbortSignal colliding with the one defined in `typescript/…

a5a6043

…lib/lib.dom.d.ts`

typescript-bot added The CI failed When GH Actions fails and removed The CI failed When GH Actions fails labels Jun 2, 2022

attempt fix #2

1ee7f13

typescript-bot added The CI failed When GH Actions fails and removed The CI failed When GH Actions fails labels Jun 2, 2022

mcollina approved these changes Jun 5, 2022

View reviewed changes

typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Jun 5, 2022

zone117x mentioned this pull request Jun 9, 2022

Consider splitting dom lib events out into separate lib for nodejs use microsoft/TypeScript#43972

Open

typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Jul 3, 2022

typescript-bot closed this Jul 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add AbortSignal.timeout(number) #60628

feat: add AbortSignal.timeout(number) #60628

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feat: add AbortSignal.timeout(number) #60628

feat: add AbortSignal.timeout(number) #60628

Uh oh!

Conversation

Uh oh!

Uh oh!

1 package in this PR

Code Reviews

Status

Inactive

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!