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

fix(fromEvent): match targets properly; fix result selector type #6208

Merged
merged 5 commits into from
Apr 9, 2021
Merged

fix(fromEvent): match targets properly; fix result selector type #6208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ea57c6a
test(fromEvent): make dtslint tests fail
cartant Apr 4, 2021
fcf7d34
fix(fromEvent): use methods not props
cartant Apr 4, 2021
ad8a36d
refactor: remove redundant optional qualifiers
cartant Apr 4, 2021
c8f7075
fix: result selector and target cannot both use T
cartant Apr 4, 2021
36a8230
chore: update api_guardian
cartant Apr 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions api_guard/dist/types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,9 @@ export declare function from<O extends ObservableInput<any>>(input: O): Observab
export declare function from<O extends ObservableInput<any>>(input: O, scheduler: SchedulerLike): Observable<ObservedValueOf<O>>;

export declare function fromEvent<T>(target: FromEventTarget<T>, eventName: string): Observable<T>;
export declare function fromEvent<T>(target: FromEventTarget<T>, eventName: string, resultSelector?: (...args: any[]) => T): Observable<T>;
export declare function fromEvent<T>(target: FromEventTarget<T>, eventName: string, options?: EventListenerOptions): Observable<T>;
export declare function fromEvent<T>(target: FromEventTarget<T>, eventName: string, options: EventListenerOptions, resultSelector: (...args: any[]) => T): Observable<T>;
export declare function fromEvent<T>(target: FromEventTarget<any>, eventName: string, resultSelector: (...args: any[]) => T): Observable<T>;
export declare function fromEvent<T>(target: FromEventTarget<T>, eventName: string, options: EventListenerOptions): Observable<T>;
export declare function fromEvent<T>(target: FromEventTarget<any>, eventName: string, options: EventListenerOptions, resultSelector: (...args: any[]) => T): Observable<T>;

export declare function fromEventPattern<T>(addHandler: (handler: NodeEventHandler) => any, removeHandler?: (handler: NodeEventHandler, signal?: any) => void): Observable<T>;
export declare function fromEventPattern<T>(addHandler: (handler: NodeEventHandler) => any, removeHandler?: (handler: NodeEventHandler, signal?: any) => void, resultSelector?: (...args: any[]) => T): Observable<T>;
Expand Down
57 changes: 44 additions & 13 deletions spec-dtslint/observables/fromEvent-spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,44 +1,75 @@
import { fromEvent } from 'rxjs';
import { JQueryStyleEventEmitter } from '../../src/internal/observable/fromEvent';
import { A, B } from '../helpers';
import {
HasEventTargetAddRemove,
NodeStyleEventEmitter,
NodeCompatibleEventEmitter,
JQueryStyleEventEmitter
} from '../../src/internal/observable/fromEvent';
import { B } from '../helpers';

declare const eventTargetSource: EventTarget;

it('should support an event target source', () => {
const source: HasEventTargetAddRemove<Event> = eventTargetSource;
const a = fromEvent(eventTargetSource, "click"); // $ExpectType Observable<Event>
});

it('should support an event target source result selector', () => {
const a = fromEvent(eventTargetSource, "click", () => "clunk"); // $ExpectType Observable<string>
});

declare const documentSource: HTMLDocument;

it('should support a document source', () => {
const source: HasEventTargetAddRemove<Event> = documentSource;
const a = fromEvent(documentSource, "click"); // $ExpectType Observable<Event>
});

interface NodeStyleSource {
addListener: (eventName: string | symbol, handler: (...args: any[]) => void) => this;
removeListener: (eventName: string | symbol, handler: (...args: any[]) => void) => this;
};
it('should support a document source result selector', () => {
const a = fromEvent(documentSource, "click", () => "clunk"); // $ExpectType Observable<string>
});

declare const nodeStyleSource : NodeStyleSource;
// Pick the parts that will match NodeStyleEventEmitter. If this isn't done, it
// will match JQueryStyleEventEmitter - because of the `on` and `off` methods -
// despite the latter being declared last in the EventTargetLike union.
declare const nodeStyleSource: Pick<typeof process, 'addListener' | 'removeListener'>;

it('should support a node-style source', () => {
const a = fromEvent(nodeStyleSource, "something"); // $ExpectType Observable<unknown>
const b = fromEvent<B>(nodeStyleSource, "something"); // $ExpectType Observable<B>
const source: NodeStyleEventEmitter = nodeStyleSource;
const a = fromEvent(nodeStyleSource, "exit"); // $ExpectType Observable<unknown>
const b = fromEvent<B>(nodeStyleSource, "exit"); // $ExpectType Observable<B>
});

it('should support a node-style source result selector', () => {
const a = fromEvent(nodeStyleSource, "exit", () => "bye"); // $ExpectType Observable<string>
});

declare const nodeCompatibleSource: {
addListener: (eventName: string, handler: (...args: any[]) => void) => void;
removeListener: (eventName: string, handler: (...args: any[]) => void) => void;
const nodeCompatibleSource = {
addListener(eventName: "something", handler: () => void) {},
removeListener(eventName: "something", handler: () => void) {}
};

it('should support a node-compatible source', () => {
const source: NodeCompatibleEventEmitter = nodeCompatibleSource;
const a = fromEvent(nodeCompatibleSource, "something"); // $ExpectType Observable<unknown>
const b = fromEvent<B>(nodeCompatibleSource, "something"); // $ExpectType Observable<B>
});

declare const jQueryStyleSource: JQueryStyleEventEmitter<A, B>;
it('should support a node-compatible source result selector', () => {
const a = fromEvent(nodeCompatibleSource, "something", () => "something else"); // $ExpectType Observable<string>
});

const jQueryStyleSource = {
on(eventName: "something", handler: (this: any, b: B) => any) {},
off(eventName: "something", handler: (this: any, b: B) => any) {}
};

it('should support a jQuery-style source', () => {
const source: JQueryStyleEventEmitter<any, any> = jQueryStyleSource;
const a = fromEvent(jQueryStyleSource, "something"); // $ExpectType Observable<B>
const b = fromEvent<B>(jQueryStyleSource, "something"); // $ExpectType Observable<B>
});

it('should support a jQuery-style source result selector', () => {
const a = fromEvent(jQueryStyleSource, "something", () => "something else"); // $ExpectType Observable<string>
});
20 changes: 10 additions & 10 deletions src/internal/observable/fromEvent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ const eventTargetMethods = ['addEventListener', 'removeEventListener'] as const;
const jqueryMethods = ['on', 'off'] as const;

export interface NodeStyleEventEmitter {
addListener: (eventName: string | symbol, handler: NodeEventHandler) => this;
removeListener: (eventName: string | symbol, handler: NodeEventHandler) => this;
addListener(eventName: string | symbol, handler: NodeEventHandler): this;
removeListener(eventName: string | symbol, handler: NodeEventHandler): this;
}

export type NodeEventHandler = (...args: any[]) => void;
Expand All @@ -21,15 +21,15 @@ export type NodeEventHandler = (...args: any[]) => void;
// not use the same arguments or return EventEmitter values
// such as React Native
export interface NodeCompatibleEventEmitter {
addListener: (eventName: string, handler: NodeEventHandler) => void | {};
removeListener: (eventName: string, handler: NodeEventHandler) => void | {};
addListener(eventName: string, handler: NodeEventHandler): void | {};
removeListener(eventName: string, handler: NodeEventHandler): void | {};
}

// Use handler types like those in @types/jquery. See:
// https://github.com/DefinitelyTyped/DefinitelyTyped/blob/847731ba1d7fa6db6b911c0e43aa0afe596e7723/types/jquery/misc.d.ts#L6395
export interface JQueryStyleEventEmitter<TContext, T> {
on: (eventName: string, handler: (this: TContext, t: T, ...args: any[]) => any) => void;
off: (eventName: string, handler: (this: TContext, t: T, ...args: any[]) => any) => void;
on(eventName: string, handler: (this: TContext, t: T, ...args: any[]) => any): void;
off(eventName: string, handler: (this: TContext, t: T, ...args: any[]) => any): void;
}

export interface EventListenerObject<E> {
Expand Down Expand Up @@ -70,11 +70,11 @@ export interface AddEventListenerOptions extends EventListenerOptions {

export function fromEvent<T>(target: FromEventTarget<T>, eventName: string): Observable<T>;
/** @deprecated resultSelector no longer supported, pipe to map instead */
export function fromEvent<T>(target: FromEventTarget<T>, eventName: string, resultSelector?: (...args: any[]) => T): Observable<T>;
export function fromEvent<T>(target: FromEventTarget<T>, eventName: string, options?: EventListenerOptions): Observable<T>;
export function fromEvent<T>(target: FromEventTarget<any>, eventName: string, resultSelector: (...args: any[]) => T): Observable<T>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to make sense. We're loosing up the type here, because really it's the T that matters. However, part of me feels like we should undeprecate this signature (as discussed in #5824), and in that case, we should try to get the types correct that are being passed to the result selector. I suppose we can do that in another pass though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. That's something that can be done after it's split into separate signatures. ATM, FromEventTarget makes the function look simpler than it really is and the T is really only applicable to some of the signatures.

export function fromEvent<T>(target: FromEventTarget<T>, eventName: string, options: EventListenerOptions): Observable<T>;
/** @deprecated resultSelector no longer supported, pipe to map instead */
export function fromEvent<T>(
target: FromEventTarget<T>,
target: FromEventTarget<any>,
eventName: string,
options: EventListenerOptions,
resultSelector: (...args: any[]) => T
Expand Down Expand Up @@ -211,7 +211,7 @@ export function fromEvent<T>(
}
if (resultSelector) {
// DEPRECATED PATH
return fromEvent<T>(target, eventName, options as EventListenerOptions | undefined).pipe(mapOneOrManyArgs(resultSelector));
return fromEvent<T>(target, eventName, options as EventListenerOptions).pipe(mapOneOrManyArgs(resultSelector));
}

// Figure out our add and remove methods. In order to do this,
Expand Down