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.

Sign up for GitHub

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

Jump to bottom

Node global interfaces redux #62782

Merged
merged 6 commits into from
Oct 18, 2022
Merged

Node global interfaces redux #62782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
180d917
[node] First pass at separate tests under lib-dom
thw0rted Oct 18, 2022
9428d92
[node] Run all 4.8 tests under lib-dom
thw0rted Oct 18, 2022
916244f
Merge branch 'master' into node-global-interfaces-again
sandersn Oct 18, 2022
436eaaf
Move DOM tests to root (covering ts4.9)
sandersn Oct 18, 2022
79cfad4
Remove added whitespace
sandersn Oct 18, 2022
06f03c5
Add missing dom-events tests
sandersn Oct 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions types/node/dom-events.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ export {}; // Don't export anything!

// This conditional type will be the existing global Event in a browser, or
// the copy below in a Node environment.
type __Event = typeof globalThis extends { onmessage: any, Event: infer T }
? T
type __Event = typeof globalThis extends { onmessage: any, Event: any }
? {}
: {
/** This is not used in Node.js and is provided purely for completeness. */
readonly bubbles: boolean;
Expand Down Expand Up @@ -48,8 +48,8 @@ type __Event = typeof globalThis extends { onmessage: any, Event: infer T }
};

// See comment above explaining conditional type
type __EventTarget = typeof globalThis extends { onmessage: any, EventTarget: infer T }
? T
type __EventTarget = typeof globalThis extends { onmessage: any, EventTarget: any }
? {}
: {
/**
* Adds a new handler for the `type` event. Any given `listener` is added only once per `type` and per `capture` option value.
Expand Down
26 changes: 15 additions & 11 deletions types/node/globals.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,21 @@ interface AbortSignal extends EventTarget {
readonly aborted: boolean;
}

declare var AbortController: {
prototype: AbortController;
new(): AbortController;
};

declare var AbortSignal: {
prototype: AbortSignal;
new(): AbortSignal;
// TODO: Add abort() static
timeout(milliseconds: number): AbortSignal;
};
declare var AbortController: typeof globalThis extends {onmessage: any; AbortController: infer T}
? T
: {
prototype: AbortController;
new(): AbortController;
};

declare var AbortSignal: typeof globalThis extends {onmessage: any; AbortSignal: infer T}
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link

@SMotaal SMotaal Dec 30, 2022 •
edited

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 🙏

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link

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.

Copy link
Contributor

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.

Copy link

@SMotaal SMotaal Jan 2, 2023 •
edited

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.

? T
: {
prototype: AbortSignal;
new(): AbortSignal;
// TODO: Add abort() static
timeout(milliseconds: number): AbortSignal;
};
//#endregion borrowed

//#region ArrayLike.at()
Expand Down
1 change: 1 addition & 0 deletions types/node/node-tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import './test/crypto';
import './test/dgram';
import './test/diagnostics_channel';
import './test/dns';
import './test/dom-events'; // dom-events behaves differently under lib-dom
import './test/events';
import './test/fs';
import './test/globals';
Expand Down
2 changes: 1 addition & 1 deletion types/node/test/buffer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ buff.writeDoubleBE(123.123, 0);

{
// $ExpectType Blob | undefined
resolveObjectURL(URL.createObjectURL(new NodeBlob([''])));
resolveObjectURL(URL.createObjectURL(new Blob([''])));
Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Member

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?

Copy link
Contributor Author

@sandersn sandersn Oct 18, 2022 •
edited

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.

}

{
Expand Down
20 changes: 20 additions & 0 deletions types/node/test/dom-events.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
//// Test global event interfaces

{
const e: Event = new Event("");
e.preventDefault();
// @ts-expect-error - ensure constructor does not return a constructor
new e();

// Node's "DOM-like" Event differs from the real DOM in a few key aspects
// $ExpectType boolean
e.cancelBubble;
e.composedPath();
// $ExpectType number
e.eventPhase;

const et: EventTarget = new EventTarget();
et.addEventListener("", () => {}, {passive: true});
// @ts-expect-error - ensure constructor does not return a constructor
new et();
}
12 changes: 2 additions & 10 deletions types/node/test/events.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,17 +121,9 @@ async function test() {

const eventTarget = new EventTarget();
events.EventEmitter.setMaxListeners(42, eventTarget);
// @ts-expect-error - ensure constructor does not return a constructor
new eventTarget();

const eventEmitter = new events.EventEmitter();
events.EventEmitter.setMaxListeners(42, eventTarget, eventEmitter);
}

{
// Some event properties differ from DOM types
const evt = new Event("fake");
evt.cancelBubble();
// @ts-expect-error
evt.composedPath[2];
// $ExpectType 0 | 2
evt.eventPhase;
}
34 changes: 7 additions & 27 deletions types/node/test/perf_hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,10 @@ import {
PerformanceEntry,
EntryType,
constants,
EventLoopUtilization,
IntervalHistogram,
RecordableHistogram,
createHistogram,
NodeGCPerformanceDetail,
PerformanceMeasure,
PerformanceMark,
} from 'node:perf_hooks';

Expand All @@ -25,24 +23,7 @@ performance.mark('test', {
startTime: 123,
});

performance.measure('test', {
detail: 'something',
duration: 123,
start: startMark.name,
end: 'endMark',
});

performance.measure('test', {
detail: 'something',
duration: 123,
start: 123,
end: 456,
});

const measure1: PerformanceMeasure = performance.measure('name', startMark.name, 'endMark');
measure1.toJSON();
performance.measure('name', startMark.name);
performance.measure('name');
performance.measure('name', startMark.name, 'endMark');

const timeOrigin: number = performance.timeOrigin;

Expand Down Expand Up @@ -89,9 +70,8 @@ const mean: number = monitor.mean;
const stddev: number = monitor.stddev;
const exceeds: number = monitor.exceeds;

const eventLoopUtilization1: EventLoopUtilization = performance.eventLoopUtilization();
const eventLoopUtilization2: EventLoopUtilization = performance.eventLoopUtilization(eventLoopUtilization1);
const eventLoopUtilization3: EventLoopUtilization = performance.eventLoopUtilization(eventLoopUtilization2, eventLoopUtilization1);
// @ts-expect-error - Node API isn't available in DOM environment
performance.eventLoopUtilization();

let histogram: RecordableHistogram = createHistogram({
figures: 123,
Expand Down Expand Up @@ -140,9 +120,9 @@ performance.clearMarks("test");
performance.clearMeasures();
performance.clearMeasures("test");

performance.getEntries(); // $ExpectType PerformanceEntry[]
performance.getEntries()[0]; // $ExpectType PerformanceEntry

performance.getEntriesByName("test"); // $ExpectType PerformanceEntry[]
performance.getEntriesByName("test", "mark"); // $ExpectType PerformanceEntry[]
performance.getEntriesByName("test")[0]; // $ExpectType PerformanceEntry
performance.getEntriesByName("test", "mark")[0]; // $ExpectType PerformanceEntry

performance.getEntriesByType("mark"); // $ExpectType PerformanceEntry[]
performance.getEntriesByType("mark")[0]; // $ExpectType PerformanceEntry
4 changes: 2 additions & 2 deletions types/node/ts4.8/buffer.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,8 @@ declare module 'buffer' {
import { Blob as NodeBlob } from 'buffer';
// This conditional type will be the existing global Blob in a browser, or
// the copy below in a Node environment.
type __Blob = typeof globalThis extends { onmessage: any, Blob: infer T }
? T : NodeBlob;
type __Blob = typeof globalThis extends { onmessage: any, Blob: any }
? {} : NodeBlob;

global {
// Buffer class
Expand Down
8 changes: 4 additions & 4 deletions types/node/ts4.8/dom-events.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ export {}; // Don't export anything!

// This conditional type will be the existing global Event in a browser, or
// the copy below in a Node environment.
type __Event = typeof globalThis extends { onmessage: any, Event: infer T }
? T
type __Event = typeof globalThis extends { onmessage: any, Event: any }
? {}
: {
/** This is not used in Node.js and is provided purely for completeness. */
readonly bubbles: boolean;
Expand Down Expand Up @@ -48,8 +48,8 @@ type __Event = typeof globalThis extends { onmessage: any, Event: infer T }
};

// See comment above explaining conditional type
type __EventTarget = typeof globalThis extends { onmessage: any, EventTarget: infer T }
? T
type __EventTarget = typeof globalThis extends { onmessage: any, EventTarget: any }
? {}
: {
/**
* Adds a new handler for the `type` event. Any given `listener` is added only once per `type` and per `capture` option value.
Expand Down
1 change: 1 addition & 0 deletions types/node/ts4.8/node-tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import './test/crypto';
import './test/dgram';
import './test/diagnostics_channel';
import './test/dns';
import './test/dom-events';
import './test/events';
import './test/fs';
import './test/globals';
Expand Down
19 changes: 19 additions & 0 deletions types/node/ts4.8/test/dom-events.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//// Test global event interfaces

{
// Some event properties differ from DOM types
const evt = new Event("fake");
evt.cancelBubble();
// @ts-expect-error
evt.composedPath[2];
// $ExpectType 0 | 2
evt.eventPhase;

// @ts-expect-error - ensure constructor does not return a constructor
new evt();

const et: EventTarget = new EventTarget();
et.addEventListener("", () => {}, {passive: true});
// @ts-expect-error - ensure constructor does not return a constructor
new et();
}
2 changes: 2 additions & 0 deletions types/node/ts4.8/test/events.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ async function test() {

const eventTarget = new EventTarget();
events.EventEmitter.setMaxListeners(42, eventTarget);
// @ts-expect-error - ensure constructor does not return a constructor
new eventTarget();

const eventEmitter = new events.EventEmitter();
events.EventEmitter.setMaxListeners(42, eventTarget, eventEmitter);
Expand Down
2 changes: 1 addition & 1 deletion types/node/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"module": "commonjs",
"target": "esnext",
"lib": [
"es6"
"es6", "dom"
peterblazejewicz marked this conversation as resolved.
Show resolved Hide resolved
],
"noImplicitAny": true,
"noImplicitThis": true,
Expand Down
4 changes: 2 additions & 2 deletions types/node/v16/stream.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/
declare module 'stream' {
import { EventEmitter, Abortable } from 'node:events';
import { Blob } from 'node:buffer';
import { Blob as NodeBlob } from 'node:buffer';
import * as streamPromises from 'node:stream/promises';
import * as streamConsumers from 'node:stream/consumers';
class internal extends EventEmitter {
Expand Down Expand Up @@ -840,7 +840,7 @@ declare module 'stream' {
*
* @since v16.8.0
*/
static from(src: Stream | Blob | ArrayBuffer | string | Iterable<any> | AsyncIterable<any> | AsyncGeneratorFunction | Promise<any> | Object): Duplex;
static from(src: Stream | NodeBlob | ArrayBuffer | string | Iterable<any> | AsyncIterable<any> | AsyncGeneratorFunction | Promise<any> | Object): Duplex;
_write(chunk: any, encoding: BufferEncoding, callback: (error?: Error | null) => void): void;
_writev?(
chunks: Array<{
Expand Down
2 changes: 2 additions & 0 deletions types/node/v16/test/events.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ async function test() {

const eventTarget = new EventTarget();
events.EventEmitter.setMaxListeners(42, eventTarget);
// @ts-expect-error - ensure constructor does not return a constructor
new eventTarget();

const eventEmitter = new events.EventEmitter();
events.EventEmitter.setMaxListeners(42, eventTarget, eventEmitter);
Expand Down
2 changes: 2 additions & 0 deletions types/node/v16/ts4.8/test/events.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ async function test() {

const eventTarget = new EventTarget();
events.EventEmitter.setMaxListeners(42, eventTarget);
// @ts-expect-error - type returned by EventTarget constuctor should not also be a constructor
new eventTarget();

const eventEmitter = new events.EventEmitter();
events.EventEmitter.setMaxListeners(42, eventTarget, eventEmitter);
Expand Down