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

Consider splitting dom lib events out into separate lib for nodejs use #43972

Open
SimonSchick opened this issue May 5, 2021 · 20 comments
Open
Assignees
Labels
Experimentation Needed Someone needs to try this out to see what happens In Discussion Not yet reached consensus Rescheduled This issue was previously scheduled to an earlier milestone Suggestion An idea for TypeScript

Comments

@SimonSchick
Copy link

lib Update Request

Configuration Check

Does not apply.

Missing / Incorrect Definition

Does not apply.

Sample Code

Does not apply.

Documentation Link

NodeJS recently implementation most of the DOM event API https://nodejs.org/dist/latest-v16.x/docs/api/events.html#events_eventtarget_and_event_api I'm currently in the process of implementing these in the node typings but this is resulting in me practically copy/pasting type definitions, we should consider splitting them out so we can re-use them.

@SimonSchick
Copy link
Author

Note:
The currently affected types are as follows (includes node changes)

interface PostMessageOptions {
    transfer?: any[];
}

interface MessageEventInit<T = any> extends EventInit {
    data?: T;
    lastEventId?: string;
    origin?: string;
    ports?: MessagePort[];
    source?: MessagePort | null;
}

interface MessagePortEventMap {
    "message": MessageEvent;
    "messageerror": MessageEvent;
}

type NodeTransferable = ArrayBuffer | MessagePort;

/** This Channel Messaging API interface represents one of the two ports of a MessageChannel, allowing messages to be sent from one port and listening out for them arriving at the other. */
interface MessagePort extends EventTarget {
    onmessage: ((this: MessagePort, ev: MessageEvent) => any) | null;
    onmessageerror: ((this: MessagePort, ev: MessageEvent) => any) | null;
    /**
     * Disconnects the port, so that it is no longer active.
     */
    close(): void;
    /**
     * Posts a message through the channel. Objects listed in transfer are transferred, not just cloned, meaning that they are no longer usable on the sending side.
     *
     * Throws a "DataCloneError" DOMException if transfer contains duplicate objects or port, or if message could not be cloned.
     */
    postMessage(message: any, transfer: NodeTransferable[]): void;
    postMessage(message: any, options?: PostMessageOptions): void;
    /**
     * Begins dispatching messages received on the port.
     */
    start(): void;
    addEventListener<K extends keyof MessagePortEventMap>(type: K, listener: (this: MessagePort, ev: MessagePortEventMap[K]) => any, options?: boolean | AddEventListenerOptions): void;
    addEventListener(type: string, listener: EventListenerOrEventListenerObject2, options?: boolean | AddEventListenerOptions): void;
    removeEventListener<K extends keyof MessagePortEventMap>(type: K, listener: (this: MessagePort, ev: MessagePortEventMap[K]) => any, options?: boolean | EventListenerOptions): void;
    removeEventListener(type: string, listener: EventListenerOrEventListenerObject2, options?: boolean | EventListenerOptions): void;
}

var MessagePort: {
    prototype: MessagePort;
    new(): MessagePort;
};

interface MessageChannel {
    /**
     * Returns the first MessagePort object.
     */
    readonly port1: MessagePort;
    /**
     * Returns the second MessagePort object.
     */
    readonly port2: MessagePort;
}

var MessageChannel: {
    prototype: MessageChannel;
    new(): MessageChannel;
};

/** A message received by a target object. */
interface MessageEvent<T = any> extends Event {
    /**
     * Returns the data of the message.
     */
    readonly data: T;
    /**
     * Returns the last event ID string, for server-sent events.
     */
    readonly lastEventId: string;
    /**
     * Returns the origin of the message, for server-sent events and cross-document messaging.
     */
    readonly origin: string;
    /**
     * Returns the MessagePort array sent with the message, for cross-document messaging and channel messaging.
     */
    readonly ports: ReadonlyArray<MessagePort>;
    /**
     * Returns the WindowProxy of the source window, for cross-document messaging, and the MessagePort being attached, in the connect event fired at SharedWorkerGlobalScope objects.
     */
    readonly source: MessagePort | null;
}

var MessageEvent: {
    prototype: MessageEvent;
    new<T>(type: string, eventInitDict?: MessageEventInit<T>): MessageEvent<T>;
};
interface EventListenerOptions {
    capture?: boolean;
}

interface AddEventListenerOptions extends EventListenerOptions {
    once?: boolean;
    passive?: boolean;
}

interface EventListener {
    (evt: Event): void;
}

interface EventListenerObject {
    handleEvent(evt: Event): void;
}

type EventListenerOrEventListenerObject2 = EventListener | EventListenerObject;

/** EventTarget is a DOM interface implemented by objects that can receive events and may have listeners for them. */
interface EventTarget {
    /**
     * Appends an event listener for events whose type attribute value is type. The callback argument sets the callback that will be invoked when the event is dispatched.
     *
     * The options argument sets listener-specific options. For compatibility this can be a boolean,
     * in which case the method behaves exactly as if the value was specified as options's capture.
     *
     * When set to true, options's capture prevents callback from being invoked when the event's eventPhase attribute value is BUBBLING_PHASE. When false (or not present),
     * callback will not be invoked when event's eventPhase attribute value is CAPTURING_PHASE. Either way, callback will be invoked if event's eventPhase attribute value is AT_TARGET.
     *
     * When set to true, options's passive indicates that the callback will not cancel the event by invoking preventDefault().
     * This is used to enable performance optimizations described in § 2.8 Observing event listeners.
     *
     * When set to true, options's once indicates that the callback will only be invoked once after which the event listener will be removed.
     *
     * The event listener is appended to target's event listener list and is not appended if it has the same type, callback, and capture.
     */
    addEventListener(type: string, listener: EventListenerOrEventListenerObject2 | null, options?: boolean | AddEventListenerOptions): void;
    /**
     * Dispatches a synthetic event event to target and returns true if either event's cancelable attribute value is false or its preventDefault() method was not invoked, and false otherwise.
     */
    dispatchEvent(event: Event): boolean;
    /**
     * Removes the event listener in target's event listener list with the same type, callback, and options.
     */
    removeEventListener(type: string, callback: EventListenerOrEventListenerObject2 | null, options?: EventListenerOptions | boolean): void;
}

var EventTarget: {
    prototype: EventTarget;
    new(): EventTarget;
};

interface EventInit {
    bubbles?: boolean;
    cancelable?: boolean;
    composed?: boolean;
}

/** An event which takes place in the DOM. */
interface Event {
    /**
     * Returns true or false depending on how event was initialized. True if event goes through its target's ancestors in reverse tree order, and false otherwise.
     */
    readonly bubbles: boolean;
    cancelBubble: boolean;
    /**
     * Returns true or false depending on how event was initialized. Its return value does not always carry meaning,
     * but true can indicate that part of the operation during which event was dispatched, can be canceled by invoking the preventDefault() method.
     */
    readonly cancelable: boolean;
    /**
     * Returns true or false depending on how event was initialized. True if event invokes listeners past a ShadowRoot node that is the root of its target, and false otherwise.
     */
    readonly composed: boolean;
    /**
     * Returns the object whose event listener's callback is currently being invoked.
     */
    readonly currentTarget: EventTarget | null;
    /**
     * Returns true if preventDefault() was invoked successfully to indicate cancelation, and false otherwise.
     */
    readonly defaultPrevented: boolean;
    /**
     * Returns the event's phase, which is one of NONE, CAPTURING_PHASE, AT_TARGET, and BUBBLING_PHASE.
     */
    readonly eventPhase: number;
    /**
     * Returns true if event was dispatched by the user agent, and false otherwise.
     */
    readonly isTrusted: boolean;
    returnValue: boolean;
    /** @deprecated */
    readonly srcElement: EventTarget | null;
    /**
     * Returns the object to which event is dispatched (its target).
     */
    readonly target: EventTarget | null;
    /**
     * Returns the event's timestamp as the number of milliseconds measured relative to the time origin.
     */
    readonly timeStamp: number;
    /**
     * Returns the type of event, e.g. "click", "hashchange", or "submit".
     */
    readonly type: string;
    /**
     * Returns the invocation target objects of event's path (objects on which listeners will be invoked),
     * except for any nodes in shadow trees of which the shadow root's mode is "closed" that are not reachable from event's currentTarget.
     */
    composedPath(): EventTarget[];
    /**
     * If invoked when the cancelable attribute value is true, and while executing a listener for the event with passive set to false,
     * signals to the operation that caused event to be dispatched that it needs to be canceled.
     */
    preventDefault(): void;
    /**
     * Invoking this method prevents event from reaching any registered event listeners after the current one finishes running and,
     * when dispatched in a tree, also prevents event from reaching any other objects.
     */
    stopImmediatePropagation(): void;
    /**
     * When dispatched in a tree, invoking this method prevents event from reaching any objects other than the current object.
     */
    stopPropagation(): void;
}

var Event: {
    prototype: Event;
    new(type: string, eventInitDict?: EventInit): Event;
    readonly AT_TARGET: number;
    readonly BUBBLING_PHASE: number;
    readonly CAPTURING_PHASE: number;
    readonly NONE: number;
};

Current compatibility issues with dom lib would be:

$ tsc
events.d.ts:107:13 - error TS2717: Subsequent property declarations must have the same type.  Property 'source' must be of type 'MessageEventSource | null | undefined', but here has type 'MessagePort | null | undefined'.

107             source?: MessagePort | null;
                ~~~~~~

  ../../../../../AppData/Roaming/npm/node_modules/typescript/lib/lib.dom.d.ts:822:5
    822     source?: MessageEventSource | null;
            ~~~~~~
    'source' was also declared here.

events.d.ts:184:22 - error TS2717: Subsequent property declarations must have the same type.  Property 'source' must be of type 'MessageEventSource | null', but here has type 'MessagePort | null'.

184             readonly source: MessagePort | null;
                         ~~~~~~

  ../../../../../AppData/Roaming/npm/node_modules/typescript/lib/lib.dom.d.ts:10359:14
    10359     readonly source: MessageEventSource | null;
                       ~~~~~~
    'source' was also declared here

The Transferable and EventListenerOrEventListenerObject types are also problematic as some of the APIs are not available within node.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels May 6, 2021
@SimonSchick
Copy link
Author

This is starting to become problematic now with DefinitelyTyped/DefinitelyTyped#52732 since the DOM event api was overhauled with generics in TS 4.0 which means I will likely have to introduce a ts4.0 folder and duplicate legacy typings in addition to the new typings which is kind of a mess TBH.

It would be great if the TS lib team can in the future keep an eye on the adaption of DOM API features in node and pre-emptively split out dom libs accordingly, the overhead for this should be fairly small (I think).

@SimonSchick
Copy link
Author

This is becoming increasingly more pressing with additions like https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V16.md#16.5.0, WASI, the URL class etc.

Can you please look into modulurazing the dom libs?

I think it would be even more beneficial now that the lib.dom.d.ts file has close to 20 kLoC

@saschanaz
Copy link
Contributor

I wonder we can have modularized version of the libs so that others can import them.

@juanrgm
Copy link

juanrgm commented Aug 30, 2021

Same case for Blob objects.

Maybe the solution for all this is:

  1. Fork index.d.ts file from @types/web into two files:
  • @types/web/index.d.ts: Export all interfaces/classes (without declarations).
  • @types/web/global.d.ts: Declare all classes/variables (imported from previous file).
  1. Replace dom library with @types/web/global (Support having '@types/web' as overriding 'dom' in TypeScript's lib resolver #44795).

In this way you can import any dom type from Node.js (@types/web/index.d.ts) and share code between environments.

@orta
Copy link
Contributor

orta commented Sep 14, 2021

Alright, I've chatted with a few TS folks about microsoft/TypeScript-DOM-lib-generator#1144 and have a better sense of what a complete answer for this looks like, given that we've hit similar problems with Array, Map and Set.

The key is to switch from:

var EventTarget: {
    prototype: EventTarget;
    new(): EventTarget;
};

to

interface EventTargetConstructor {
    prototype: EventTarget;
    new(): EventTarget;
}

var EventTarget: EventTargetConstructor

Which safely allows for subsequent re-imports and extensions. It's how we handle it in all of the JS primitives.

I think our answer here is to switch out the creation of the literal in the dom dts gen, and then also introduce a new file in the TypeScript lib files which contains the types + vars which we think should be shared (so that @types/node can import it via /// <reference...)

@saschanaz
Copy link
Contributor

That recalls me my old comment: #39054 (comment)

It means if you type EventHandler you'll get a suggestion for EventHandlerStatic (or whatever) and this will happen for every interface name.

Is this an acceptable UX tradeoff? 🤔

Also, was there any opposition against the module idea? I'm not sure it's ideal to import everything only for certain wanted types.

@orta
Copy link
Contributor

orta commented Sep 14, 2021

Perhaps we use StaticEventHandler or something so the auto-complete is rarely seen. But we live with ArrayConstructor today, and people use Array all the time, so I think it should be fine. I wouldn't be surprised if we reduce the ranking of an 'XConstructor' in the results specifically for this (given the maturity of TS)

Modules can't really work without forcing everyone to use npm modules for dom libs, because there's no module specifier to refer to which resolves to a tslib. If we separated out the shared types into a shared npm lib, then people have to understand that they could have three differing sets of type sources if they have node and dom types in the same space that all have different versioning/releasing strategies:

  • @types/node whenever DT does it
  • @types/dom-shared whenever dom-lib-gen does it
  • dom libs or @types/web whenever TS updates, or you lock it (+ whenever dom lib does it)

I'd much rather try keep that down to two place which could change, because each is a potential place to get out of sync with the others.

That said, we don't want all of the dom types+globals available in the shared lib either - it'd just be the common ones (and maybe what we think Node will want in the near future too)

@saschanaz
Copy link
Contributor

saschanaz commented Sep 14, 2021

there's no module specifier to refer to which resolves to a tslib

Isn't it able to be done in #45771, although it would require users to use newer TS?

  • @types/dom-shared whenever dom-lib-gen does it
  • dom libs or @types/web whenever TS updates, or you lock it (+ whenever dom lib does it)

I think these two could only have same versioning stratagies when they would be generated by the same code...

That said, we don't want all of the dom types+globals available in the shared lib either - it'd just be the common ones (and maybe what we think Node will want in the near future too)

"common ones"ᅟ or "all types referenced by any non-browser environments"? I doubt there will be dom-shared dedicated for Node and another for Deno, so I guess it's the latter. It could be confusing to have things only Deno have when a user installed only @types/node. I can imagine bug reports about it 👀

@saschanaz
Copy link
Contributor

Perhaps we use StaticEventHandler or something so the auto-complete is rarely seen. But we live with ArrayConstructor today, and people use Array all the time, so I think it should be fine. I wouldn't be surprised if we reduce the ranking of an 'XConstructor' in the results specifically for this (given the maturity of TS)

Cool enough. I personally prefer classes but well that would sadly be a breaking change as it can't coexist with vars...

@jimmywarting
Copy link
Contributor

jimmywarting commented Nov 2, 2021

  • @types/web/index.d.ts: Export all interfaces/classes (without declarations).
  • @types/web/global.d.ts: Declare all classes/variables (imported from previous file).

I like this, could the rule for something to land in global be that Deno, Node and Browser, (possible some web worker threads) all have it globally (not just web dom and node)

@jimmywarting
Copy link
Contributor

DOMException was now also exposed globally in NodeJS

@orta
Copy link
Contributor

orta commented Dec 6, 2021

I've tried making a usable prototype of technique mentioned above in #43972 (comment) which can be tested out with @orta/lib-dom-globals.

I need to test it out on a few projects to get a sense of whether this works as expected, and go through it with some other TS folks but I'd love it if others could take a look too

https://github.com/orta/orta-types-web-globals/

A version of @types/web which has the types separated from the declaration of globals based on this diff with a bit of manual hand-holding by me.

Based on discussion in microsoft/TypeScript#43972.

Old:

var EventTarget: {
    prototype: EventTarget;
    new(): EventTarget;
};

Now:

interface EventTargetConstructor {
    prototype: EventTarget;
    new(): EventTarget;
}

var EventTarget: EventTargetConstructor

This library can be used like:

pnpm add @typescript/lib-dom@npm:@orta/lib-dom-globals --save-dev
npm install @typescript/lib-dom@npm:@orta/lib-dom-globals --save-dev
yarn add @typescript/lib-dom@npm:@orta/lib-dom-globals --dev

Which will give other projects like node/deno etc the ability to do:

/// <reference lib="dom.types" />

Which resolves to ./types.d.ts - which only contains the global type and no runtime declarations.

Example App

Here's an example of what this would look like in practice:

@saschanaz
Copy link
Contributor

saschanaz commented Dec 6, 2021

With modular lib.d.ts it can also be:

// in node/index.d.ts:
declare var EventTarget: typeof import("typescript/dom-modular").EventTarget;

This way there won't be tons of additional names in the global namespace. (That would require newer version of typescript for all node.d.ts consumers 🤔, but same applies to the *Constructor way)

@zone117x
Copy link

zone117x commented Jun 9, 2022

Are there any recommended workarounds for this issue right now? NodeJS is adding more Web APIs as globals, and they seem quite difficult to implement in @types/node. For example simply adding a property to AbortSignal here: DefinitelyTyped/DefinitelyTyped#60628

Another big upcoming addition will be the fetch Web APIs, which I suspect will run into similar issues.

@thw0rted
Copy link

thw0rted commented Aug 2, 2022

I think anyone following this issue is likely to take an interest in this PR I've been working on. The unfortunate conclusion that I've reached is that Node is introducing incompatible near-clones of DOM types.

When I filed microsoft/TypeScript-DOM-lib-generator#1207, the idea was that Node's global Blob would just be a copy/paste of the lib-dom Blob type. That isn't true, because Node's Blob#stream() method returns an instance of ReadableStream from node:stream/web, which is not actually compatible with the ReadableStream provided by lib.dom.d.ts. (The Node one adds async-iterator stuff that isn't in lib-dom.) This is just one example, but something like half of the global "DOM compatible" types aren't actually quite compatible, for similar reasons. It gets even worse when you start looking at anything with Events, because you eventually wind up incorporating Node's EventEmitter, which supports node-isms like thing.on("message", () => { ... }) that aren't present in lib-dom.

So: I'm increasingly convinced that trying to maintain one set of "DOM types" that can be incorporated in both the browser environment and in server-side runtimes that support a subset of them at the global scope is simply not going to work. That's why I'm flailing in the linked PR. If we can't have 100% guaranteed compatible interfaces in both @types/node and lib.dom.d.ts, there are going to be conflicts when both are included -- and both will be included in a lot of projects, as dozens of issues filed here over the past few years show us -- then the only solution I could come up with is to break @types/node, pretending that their globally scoped "compatible" types do not exist. Anything else would require a way to conditionally declare/augment global interfaces, and AFAICT that is not possible. (Please, please correct me if I'm wrong!)

@jimmywarting
Copy link
Contributor

jimmywarting commented Aug 2, 2022

Async iterator is part of the readable stream specification. It just happened to be that none of the browser have implemented it yet.
I don't think that should be a blocker cuz it's not supported on the browsers. It's more annoying that we have 2 sets of libs and need to maintain both. I think that async iterators should rather be polyfilled if needed

I do not think you should different the NodeJS web streams as something that is node specific.
NodeJS web streams should just be a copy of the same browsers stream specification. So either none gets support for async iterators or both of the environments gets support for async iterator.
Just consider NodeJS as another browser that have bleeding edge functionality.

Just b/c Firefox or safari have something browser specific shouldn't mean that it should be included in lib.dom.d.ts so NodeJS should not be treated any different in this regards.

I also think that Deno and cloud flare workers should be taken into consideration as well. It also includes a web spec compatible stream implementation that also supports async iterator

@thw0rted
Copy link

thw0rted commented Aug 2, 2022

Re: experimental / unsupported APIs, AFAIK the lib generator project has their own metric for when an API gets included in lib-dom.

Re: replacing node:stream/web, Blob, and whatnot, maybe it really would be OK to drop the lib-dom Blob (and thus also ReadableStream etc etc) into Node, but I still think there will be other intentionally-incompatible global types added over time. At the very least, I don't like the idea that we have to trust Node will never give us global objects that directly conflict with the "default" lib (you get dom if you don't explicitly opt out).

@thw0rted
Copy link

thw0rted commented Aug 2, 2022

It's been a few months since I looked at this so it took me some time to dig up a really-truly-incompatible Node DOM-like type. The best I've found so far is in their "web streams".

For background: you read out of a ReadableStream using getReader(), which gives you a Reader interface. That populates a "data view" either chunk-wise or byte-wise. Byte-wise reading can use typed arrays that offer a performance benefit -- the getReader call returns a "BYOB" (bring your own bytes) Reader in this case. The thing is, the web version expects a typed array (e.g. Uint8Array) or a DataView, but Node's BYOB Reader can also accept a native Buffer.

So, if we deleted node:stream/web, removed the Blob in node:buffer, and we got the @types/web package we're asking for in this issue, then @types/node could import {Blob, ReadableStream, etc, etc} from "@types/web", expose them globally, and everybody would be happy, right up until somebody tried to call myBlob.stream().getReader().read(myNativeBuffer), which would fail.

Like I said above: conflict is inevitable, and I think the TS team is going to have to get involved to fix it.

@pimterry
Copy link
Contributor

I'm hitting issues with typing a lib for both Node & browsers that's using web streams and I ran into this. I think it's worth noting that the concrete example above from @thw0rted isn't correct though, specifically:

The thing is, the web version expects a typed array (e.g. Uint8Array) or a DataView, but Node's BYOB Reader can also accept a native Buffer.

A native Buffer is a typed array. Node's buffer extends Uint8Array (2nd paragraph here: https://nodejs.org/api/buffer.html#buffer). myBlob.stream().getReader().read(myNativeBuffer) will never actually fail and should type check regardless - any native buffer is also a valid Uint8Array.

In general, my understanding is that the Node team's goal for these features is real web compatibility. The only intentional differences should be widening types (allowing more input types and/or restricting certain return types) in ways that should be entirely compatible, and anything else is a bug. That doesn't solve the problem of course, but I don't think direct conflicts are the hard part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experimentation Needed Someone needs to try this out to see what happens In Discussion Not yet reached consensus Rescheduled This issue was previously scheduled to an earlier milestone Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

10 participants