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

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

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 typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Review in New Pull Request Status Board Jan 18, 2022
@typescript-bot typescript-bot moved this from Needs Maintainer Review to Waiting for Code Reviews in New Pull Request Status Board Jan 18, 2022
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Review in New Pull Request Status Board Jan 18, 2022
@antongolub antongolub marked this pull request as draft January 18, 2022 20:19
@typescript-bot typescript-bot moved this from Needs Maintainer Review to Needs Author Action in New Pull Request Status Board Jan 19, 2022
@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
@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Jan 19, 2022
@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Jan 19, 2022
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board 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.

@typescript-bot typescript-bot removed the The CI failed When GH Actions fails label Jan 20, 2022
@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Jan 20, 2022
@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Jan 20, 2022
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board 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
@typescript-bot typescript-bot removed the The CI failed When GH Actions fails label Jan 20, 2022
@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Jan 20, 2022
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Review in New Pull Request Status Board Jan 20, 2022
@typescript-bot typescript-bot removed 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 moved this from Waiting for Code Reviews to Waiting for Author to Merge in New Pull Request Status Board Jan 31, 2022
@antongolub
Copy link
Contributor Author

Ready to merge

@typescript-bot typescript-bot moved this from Waiting for Author to Merge to Recently Merged in New Pull Request Status Board Feb 1, 2022
@typescript-bot typescript-bot merged commit 781162d into DefinitelyTyped:master Feb 1, 2022
@typescript-bot typescript-bot removed this from Recently Merged in New Pull Request Status Board Feb 1, 2022
antongolub added a commit to antongolub-forks/DefinitelyTyped that referenced this pull request Feb 2, 2022
antongolub added a commit to antongolub-forks/DefinitelyTyped that referenced this pull request Feb 3, 2022
@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