Skip to content

Conversation

@antongolub
Copy link
Contributor

@antongolub antongolub commented Jan 18, 2022

closes: #34960

NodeJS spec:

URL v10.0.0 | The class is now available on the global object.
URLSearchParams v10.0.0 | The class is now available on the global object.

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: <>

@typescript-bot
Copy link
Contributor

typescript-bot commented Jan 18, 2022

@antongolub Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

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

Status

  • ✅ No merge conflicts
  • ✅ Continuous integration tests have passed
  • ✅ Most recent commit is approved by type definition owners or DT maintainers

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": 58277,
  "author": "antongolub",
  "headCommitOid": "5f9bcb8fe3c2f9aedc05c9dadc20ee7157f14838",
  "mergeBaseOid": "ba9c5ca17fd39d9e7e3ce89be9fafd6b1bc9e24f",
  "lastPushDate": "2022-01-20T13:03:36.000Z",
  "lastActivityDate": "2022-02-01T08:07:38.000Z",
  "maintainerBlessed": "Waiting for Code Reviews",
  "mergeOfferDate": "2022-01-31T20:07:31.000Z",
  "mergeRequestDate": "2022-02-01T08:07:38.000Z",
  "mergeRequestUser": "antongolub",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/test/url.ts",
          "kind": "test"
        },
        {
          "path": "types/node/url.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v12/url.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v14/test/url.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v14/url.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/test/url.ts",
          "kind": "test"
        },
        {
          "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",
        "westy92",
        "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": "peterblazejewicz",
      "date": "2022-01-31T20:06:52.000Z",
      "isMaintainer": true
    }
  ],
  "mainBotCommentID": 1015556159,
  "ciResult": "pass"
}

@typescript-bot
Copy link
Contributor

@antongolub antongolub marked this pull request as draft January 18, 2022 20:19
@demurgos
Copy link
Contributor

Thank you very much for sending a PR.

The dom lib defines the global first and then re-exports it through the module. This implementation does the reverse. As long as there are no conflicts I assume it should be fine.

I am really excited for this fix: it will be very helpful!

@antongolub antongolub marked this pull request as ready for review January 19, 2022 22:16
@antongolub antongolub force-pushed the node-expose-url-globals branch from 78783ea to 0ac59f6 Compare January 19, 2022 22:16
@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Jan 19, 2022
@typescript-bot
Copy link
Contributor

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

@antongolub antongolub force-pushed the node-expose-url-globals branch from 0ac59f6 to 6a9e8e4 Compare January 20, 2022 05:37
@typescript-bot typescript-bot added The CI failed When GH Actions fails and removed The CI failed When GH Actions fails labels Jan 20, 2022
@typescript-bot
Copy link
Contributor

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

@antongolub
Copy link
Contributor Author

antongolub commented Jan 20, 2022

image

TS lib.dom.d.ts "version": "4.6.0-dev.20220119"

declare var AbortSignal: {
    prototype: AbortSignal;
    new(): AbortSignal;
    abort(reason?: any): AbortSignal;
};

"version": "4.6.0-dev.20220117"

declare var AbortSignal: {
    prototype: AbortSignal;
    new(): AbortSignal;
    // abort(): AbortSignal; - To be re-added in the future
};

microsoft/TypeScript#47507

UPD
Fixed in 4.6.0-dev.20220120

@antongolub antongolub mentioned this pull request Jan 20, 2022
8 tasks
@antongolub antongolub force-pushed the node-expose-url-globals branch from 6a9e8e4 to 3428cc4 Compare January 20, 2022 10:09
@typescript-bot typescript-bot removed the The CI failed When GH Actions fails label Jan 20, 2022
@antongolub antongolub force-pushed the node-expose-url-globals branch from 3428cc4 to 5f9bcb8 Compare January 20, 2022 13:03
@antongolub
Copy link
Contributor Author

CC @SimonSchick

@gabritto
Copy link
Contributor

@typescript-bot
Copy link
Contributor

@typescript-bot typescript-bot added the Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. label Jan 31, 2022
@typescript-bot typescript-bot added Self Merge This PR can now be self-merged by the PR author or an owner and removed Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. labels Jan 31, 2022
@antongolub
Copy link
Contributor Author

Ready to merge

@sandersn
Copy link
Contributor

sandersn commented Feb 4, 2022

Note: This breaks configs that have lib: ["webworker"] because lib.webworker.d.ts includes both URL and URLSearchParams. sparql-http-client on DT is one example.

@peterblazejewicz
Copy link
Member

@sandersn what's the option here we can pick? There are two posts on discussion with same (or similar) problem already

@antongolub
Copy link
Contributor Author

antongolub commented Feb 5, 2022

Interesting...

createObjectURL(object: any): string;
createObjectURL(obj: Blob | MediaSource): string;

more and more interesting:
https://developer.mozilla.org/en-US/docs/Web/API/URL/createObjectURL
image

@antongolub
Copy link
Contributor Author

antongolub commented Feb 5, 2022

Yet another janky trick:

var URL:
    // For compatibility with "dom" and "webworker" URL declarations
    typeof globalThis extends ({ webkitURL: infer URL } | { TransformStreamDefaultController: infer T, URL: infer URL })
    ? URL
    : typeof _URL;

@sandersn, could you check this plz?

@peterblazejewicz
Copy link
Member

peterblazejewicz commented Feb 5, 2022

afaik it does not work with use case (from your PR branch):

npm test sparql-http-client

(the DT lib declaring webworker lib in tsconfig:

Error: Errors in typescript@3.8 for external dependencies:
../node/url.d.ts(872,13): error TS2403: Subsequent variable declarations must have the same type.  Variable 'URL' must be of type '{ new (url: string, base?: string | URL | undefined): URL; prototype: URL; createObjectURL(object: any): string; revokeObjectURL(url: string): void; }', but here has type 'typeof URL'.

@antongolub
Copy link
Contributor Author

Oops... 3.8 does not provide TransformStreamDefaultController var declaration. Сontinue digging.

@antongolub
Copy link
Contributor Author

@peterblazejewicz, how about?

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

@peterblazejewicz
Copy link
Member

Yes, this passes the use-case test, but let's have more opinion on what next.

antongolub added a commit to antongolub-forks/DefinitelyTyped that referenced this pull request Feb 5, 2022
@antongolub
Copy link
Contributor Author

#58619

typescript-bot pushed a commit that referenced this pull request Feb 14, 2022
… "webworker" by @antongolub

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

relates #58277 #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
martin-badin pushed a commit to martin-badin/DefinitelyTyped that referenced this pull request Feb 23, 2022
…hParams as globals by @antongolub

* feat(node): declare URL and URLSearchParams as globals

closes: DefinitelyTyped#34960

* chore(node): simplify URL and URLSearchParams global definitions

* docs(node): add @SInCE comments for URL and URLSearchParam globals

* refactor(node): tweak up URL global type after tsc manual tests

* docs(node): minor jsdoc improvements for global URL and URLSearchParams
martin-badin pushed a commit to martin-badin/DefinitelyTyped that referenced this pull request Feb 23, 2022
…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
@thw0rted
Copy link
Contributor

Anybody that's still watching this, maybe you could weigh in on #59905 where I'm trying to do something similar with another Node "DOM-compatible" API? I'm going to try to use the same pattern you discovered here, if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Critical package Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[node] Missing global URL and URLSearchParams typings

7 participants