-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
Node global interfaces redux #62782
Node global interfaces redux #62782
Conversation
Made "bogus" ts-version subdirectory
Remove ts4.7 directory and move one or two node-specific tests from root to ts4.8. This provides less test coverage; there is no exact duplicate of the types being tested. But 4.8 and 4.9 are almost identical and 4.8 is supposed to get every change that 4.9 does. So I think that the DOM-specific in 4.9 and non-DOM-specific tests in 4.9 will provide high-enough quality of coverage.
@sandersn Thank you for submitting this PR! This is a live comment which I will keep updated. 1 package in this PRCode ReviewsBecause 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
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": 62782,
"author": "sandersn",
"headCommitOid": "06f03c57e3096e44901c69d1fd83f75b269df62b",
"mergeBaseOid": "600b32ae45734942dafca38182775fdc97462760",
"lastPushDate": "2022-10-18T18:28:23.000Z",
"lastActivityDate": "2022-10-18T20:39:41.000Z",
"hasMergeConflict": false,
"isFirstContribution": false,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": "node",
"kind": "edit",
"files": [
{
"path": "types/node/dom-events.d.ts",
"kind": "definition"
},
{
"path": "types/node/globals.d.ts",
"kind": "definition"
},
{
"path": "types/node/node-tests.ts",
"kind": "test"
},
{
"path": "types/node/test/buffer.ts",
"kind": "test"
},
{
"path": "types/node/test/dom-events.ts",
"kind": "test"
},
{
"path": "types/node/test/events.ts",
"kind": "test"
},
{
"path": "types/node/test/perf_hooks.ts",
"kind": "test"
},
{
"path": "types/node/ts4.8/buffer.d.ts",
"kind": "definition"
},
{
"path": "types/node/ts4.8/dom-events.d.ts",
"kind": "definition"
},
{
"path": "types/node/ts4.8/node-tests.ts",
"kind": "test"
},
{
"path": "types/node/ts4.8/test/dom-events.ts",
"kind": "test"
},
{
"path": "types/node/ts4.8/test/events.ts",
"kind": "test"
},
{
"path": "types/node/tsconfig.json",
"kind": "package-meta-ok"
},
{
"path": "types/node/v16/stream.d.ts",
"kind": "definition"
},
{
"path": "types/node/v16/test/events.ts",
"kind": "test"
},
{
"path": "types/node/v16/ts4.8/test/events.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": "andrewbranch",
"date": "2022-10-18T20:39:41.000Z",
"isMaintainer": true
}
],
"mainBotCommentID": 1282829419,
"ciResult": "pass"
} |
🔔 @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 |
@sandersn 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. |
new(): AbortController; | ||
}; | ||
|
||
declare var AbortSignal: typeof globalThis extends {onmessage: any; AbortSignal: infer T} |
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.
Were these just missed in the original PR? Or are they new in 4.9’s lib.dom.d.ts?
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 can only answer the second part: AbortSignal is not new in 4.9, but it changed to add timeout
(and once this is fixed, I'm going to add abort
in 4.9 as well in a few days).
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.
From discussion with @andrewbranch, it seems that @thw0rted answered the first question earlier in the typescript discord: it was indeed missed in the original PR, because it wasn't tested until now.
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.
@sandersn Can you elaborate further on the source of this pattern please?
I am specifically interested in this pattern:
// @filename: node_modules/y/globals.d.ts
declare var y: typeof globalThis extends { x: any; y: infer T; } ? T : Y;
// @filename: src/index.ts
import 'y';
I am curious as to the origin of this pattern, which I traced to #34960 (comment) by @forivall.
Thanks 🙏
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 got it from @thw0rted, where it was the first I had seen it. But it sounds like you've already done more detective work than I have.
I wish it wasn't necessary but it does help the types keep working even when certain globals are not defined.
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 can't take credit -- if you look back at the original "remove DOM typings" PR, I link to a previous change to the Node typings that already used the same construct. I agree that it's not the great but unfortunately efforts to provide first-class language support for this kind of thing seem to have stalled out repeatedly over the years.
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.
Conditional types are first-class support, for lots of kludgey things. =P
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.
@thw0rted, I appreciate the clarity, but from my end, I am explicitly trying to look at the history of this to determine how stable it would be down the road for different use cases.
If there should be first-class support for this pattern or some other feature as a special case where module import order would augment the properties of globalThis
, this needs more work on TypeScript's end, at least test cases to ensure the behaviour does not break.
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 don't think it's possible for import order to change the result, because the compiler would complain if the definitions created a circular reference, but I'm not confident in my analysis and would love to see smarter people than me really kick the tires with testing.
I also don't really want to see this pattern proliferate and take hold as a standard, because frankly it's pretty hard to reason about or even read and understand. I think there must be more elegant solutions to the problems we're trying to solve -- typings for idempotent libraries, and avoiding global scope pollution when frontend libraries and server-side tooling are installed at the same time -- but I don't personally have a cure-all to advocate for instead. This seemed like the least-worst compromise.
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.
@thw0rted for reference, the earliest mention on a pull request was in #56163 (comment) from last year, but the earliest mention I found was #34960 (comment).
I agree with your points, and I think first TypeScript needs to settle on a way to define globalThis
properties within modules and there needs to be mechanisms to model this behaviour at runtime when transpiling across module formats that would be deterministic.
@@ -423,7 +423,7 @@ buff.writeDoubleBE(123.123, 0); | |||
|
|||
{ | |||
// $ExpectType Blob | undefined | |||
resolveObjectURL(URL.createObjectURL(new NodeBlob(['']))); | |||
resolveObjectURL(URL.createObjectURL(new Blob(['']))); |
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.
Why did this one change?
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.
This is a personal preference of mine, I don't like imports with names that shadow globals. (It can hide problems especially when the globals might be different as in this case.) If there's any problem with it you're more than welcome to change it back.
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.
Blob as NodeBlob
is imported in this file and the other references in the test file use NodeBlob
, so I just wasn’t sure why this one specifically is changing to Blob
?
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.
With "dom"
included, URL.createObjectUrl
comes from the DOM, and requires a DOM Blob. A node Blob does not work.
I don't know if it should work, but the two types are incompatible, so I changed this test to show that. Once the types are not broken, we can figure out if the two Blob types should be compatible.
Most of the work here is from @thw0rted ; I then switched the DOM tests to the root, which covers ts4.9.
The root's tsconfig now includes
"dom"
, so its tests are meant to cover projects that include both@types/node
and"dom"
. ts4.8/'s tsconfig still does not include"dom", and its tests are meant to cover projects that include only
@types/node`.Follow up to #59905 and #62629
Addresses #62759
May obsolete #62729