fix(node): make global URL decl compatible with lib "webworker" #58619

antongolub · 2022-02-05T08:27:06Z

relates #58277 #34960

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.)

CC @sandersn, @SimonSchick

typescript-bot · 2022-02-05T08:27:58Z

@antongolub 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

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 passed
✅ Only a DT maintainer can approve changes without tests

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 58619,
  "author": "antongolub",
  "headCommitOid": "aded305520919755420981d6a6e45c16af07bc65",
  "mergeBaseOid": "00dc9f1849e5a8a43a662bcb8add51e22135e3c0",
  "lastPushDate": "2022-02-10T06:08:26.000Z",
  "lastActivityDate": "2022-02-14T19:05:18.000Z",
  "mergeOfferDate": "2022-02-14T18:49:50.000Z",
  "mergeRequestDate": "2022-02-14T19:05:18.000Z",
  "mergeRequestUser": "antongolub",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/url.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v12/url.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v14/url.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/url.d.ts",
          "kind": "definition"
        }
      ],
      "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"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "rbuckton",
      "date": "2022-02-14T18:49:13.000Z",
      "isMaintainer": true
    },
    {
      "type": "stale",
      "reviewer": "sandersn",
      "date": "2022-02-07T18:56:16.000Z",
      "abbrOid": "ba3ac37"
    }
  ],
  "mainBotCommentID": 1030577013,
  "ciResult": "pass"
}

typescript-bot · 2022-02-05T08:28:00Z

Hey @antongolub,

😒 Your PR doesn't modify any tests, so it's hard to know what's being fixed, and your changes might regress in the future. Please consider adding tests to cover the change you're making. Including tests allows this PR to be merged by yourself and the owners of this module.

This can potentially save days of time for you!

typescript-bot · 2022-02-05T08:28:00Z

🔔 @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 — 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-02-05T18:37:30Z

@antongolub 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.

relates DefinitelyTyped#58277 DefinitelyTyped#34960

sandersn

I don't like it, but it seems necessary to get this working everywhere.

I'd sign off, but I want to see if some node types maintainers can comment as well.

…way as for URL

antongolub · 2022-02-07T20:52:57Z

I look to the future with optimism: TS is being improved step by step, day by day and over time we will be able to remove all these workarounds.

typescript-bot · 2022-02-07T20:58:50Z

@sandersn Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

rbuckton · 2022-02-09T20:59:44Z

types/node/url.d.ts

-        var URL: typeof globalThis extends { webkitURL: infer URL } ? URL : typeof _URL;
+        var URL:
+            // For compatibility with "dom" and "webworker" URL declarations
+            typeof globalThis extends { onmessage: infer O, URL: infer URL }


Isn't this circular? Declaring var URL in a global augmentation means that globalThis will have a URL even if it isn't defined elsewhere, so you might as well just be testing for { onmessage: any }. The previous check tested the presence of webkitURL, which would not be circular.

Also, why are you using infer O? The resulting type variable isn't used anywhere else.

Seems that there's no infinite recursion for URL because the onmessage type check goes first. Should I replace the current one with smth like this?

var URL: // For compatibility with "dom" and "webworker" URL declarations typeof globalThis extends { onmessage: any } ? typeof globalThis.URL : typeof _URL;

removed redundant infer O

typeof globalThis.URL is still circular, because it just references the URL you've defined if nothing else is present. The previous code could use webkitURL because it was a different declaration. Unfortunately, lib.webworker.generated.d.ts doesn't have a different definition of URL to infer from, though maybe you could do something like this:

var URL: // for compatibility with "dom" URL typeof globalThis extends { webkitURL: infer URL } ? URL : // for compatibility with "webworker" URL typeof globalThis extends { WebSocket: new (url: string | infer URL) => any } ? URL : typeof _URL;

Though the WebSocket variant could work for both dom and webworker.

ok, if typeof globalThis.URL is still circular, why tsc does not raise an error (error TS2502: 'URL' is referenced directly or indirectly in its own type annotation.)? Doesn't that mean, it refers to the URL defined by lib.dom/lib.webworker?

Honestly, I'm surprised it's not an error. If you remove "dom" from the "lib" section in the tsconfig and change the type to var URL: typeof globalThis.URL then it is an error, but the conditional type keeps that from happening.

…arams

typescript-bot · 2022-02-10T06:48:01Z

@rbuckton, @sandersn Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

typescript-bot · 2022-02-14T18:49:50Z

@antongolub: Everything looks good here. I am ready to merge this PR (at aded305) on your behalf whenever you think it's ready.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

(@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: you can do this too.)

antongolub · 2022-02-14T19:05:18Z

Ready to merge

DefinitelyTyped/DefinitelyTyped#34960 DefinitelyTyped/DefinitelyTyped#58277 DefinitelyTyped/DefinitelyTyped#58619

@antongolub

…atible with lib "webworker" by @antongolub * fix(node): make global ULR decl compatible with lib "webworker" relates DefinitelyTyped#58277 DefinitelyTyped#34960 * fix(node): improve global URL var type resolution * refactor(node): handle URLSeachParams var type collision in the same way as for URL * refactor(node): simplify type condition for global URL and URLSearchParams

antongolub requested a review from peterblazejewicz as a code owner February 5, 2022 08:27

typescript-bot added Critical package Untested Change This PR does not touch tests labels Feb 5, 2022

typescript-bot added the The CI failed When GH Actions fails label Feb 5, 2022

antongolub added 2 commits February 5, 2022 21:40

fix(node): make global ULR decl compatible with lib "webworker"

a37faf0

relates DefinitelyTyped#58277 DefinitelyTyped#34960

fix(node): improve global URL var type resolution

ba3ac37

antongolub force-pushed the fix-node-global-url-for-lib-webworker branch from e0006d8 to ba3ac37 Compare February 5, 2022 18:42

typescript-bot removed the The CI failed When GH Actions fails label Feb 5, 2022

antongolub mentioned this pull request Feb 5, 2022

feat(node): declare URL and URLSearchParams as globals #58277

Merged

7 tasks

sandersn reviewed Feb 7, 2022

View reviewed changes

refactor(node): handle URLSeachParams var type collision in the same …

59e73c9

…way as for URL

rbuckton changed the title ~~fix(node): make global ULR decl compatible with lib "webworker"~~ fix(node): make global URL decl compatible with lib "webworker" Feb 9, 2022

rbuckton reviewed Feb 9, 2022

View reviewed changes

refactor(node): simplify type condition for global URL and URLSearchP…

aded305

…arams

rbuckton approved these changes Feb 14, 2022

View reviewed changes

typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner labels Feb 14, 2022

typescript-bot merged commit 118a383 into DefinitelyTyped:master Feb 14, 2022

antongolub mentioned this pull request Feb 15, 2022

bug(node): a combination of @types/node 17.0.14 and TS 3.7.3 raises an error #58562

Closed

antongolub added a commit to antongolub-forks/globby that referenced this pull request Feb 18, 2022

chore: remove URL type workaround

ff68187

DefinitelyTyped/DefinitelyTyped#34960 DefinitelyTyped/DefinitelyTyped#58277 DefinitelyTyped/DefinitelyTyped#58619

antongolub mentioned this pull request Feb 18, 2022

chore: remove URL type workaround sindresorhus/globby#230

Merged

zone117x mentioned this pull request Jun 6, 2022

feat: add AbortSignal.timeout(number) #60628

Closed

8 tasks

fix(node): make global URL decl compatible with lib "webworker" #58619

fix(node): make global URL decl compatible with lib "webworker" #58619

Uh oh!

Conversation

antongolub commented Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1 package in this PR

Code Reviews

Status

Uh oh!

typescript-bot commented Feb 5, 2022

Uh oh!

typescript-bot commented Feb 5, 2022

Uh oh!

typescript-bot commented Feb 5, 2022

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

antongolub commented Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Feb 7, 2022

Uh oh!

rbuckton Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

antongolub Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antongolub Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

rbuckton Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

antongolub Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbuckton Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Feb 10, 2022

Uh oh!

typescript-bot commented Feb 14, 2022

Uh oh!

antongolub commented Feb 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

antongolub commented Feb 5, 2022 •

edited

Loading

typescript-bot commented Feb 5, 2022 •

edited

Loading

antongolub commented Feb 7, 2022 •

edited

Loading

antongolub Feb 10, 2022 •

edited

Loading

antongolub Feb 14, 2022 •

edited

Loading