Skip to content

Fix TS definition of publish & multicast#2616

Closed
hermanbanken wants to merge 1 commit intoReactiveX:masterfrom
hermanbanken:patch-1
Closed

Fix TS definition of publish & multicast#2616
hermanbanken wants to merge 1 commit intoReactiveX:masterfrom
hermanbanken:patch-1

Conversation

@hermanbanken
Copy link
Contributor

Description:
Change TS definition of publish & multicast to support selectors changing the type.

Related issue (if exists):
#1905

…ector

The TypeScript defintions for publish and multicast did not allow for changing the type
@hermanbanken
Copy link
Contributor Author

hermanbanken commented May 26, 2017

Currently working around like this...

declare module "rxjs/operator/publish" {
	export type selectors<T,R> = (source: Observable<T>) => Observable<R>;
	export function publish<T,R>(this: Observable<T>, selector: selectors<T,R>): Observable<R>;
}

@kwonoj kwonoj added the TS Issues and PRs related purely to TypeScript issues label May 26, 2017
@hermanbanken
Copy link
Contributor Author

Checks are failing due to some conflicts in the Travis <-> Github API communication after I fixed the commit message and force pushed... /cc @kwonoj

@coveralls
Copy link

coveralls commented May 29, 2017

Coverage Status

Coverage remained the same at 97.735% when pulling 9538f62 on hermanbanken:patch-1 into da96b6a on ReactiveX:master.

@kwonoj kwonoj requested a review from david-driscoll May 29, 2017 17:36
@kwonoj
Copy link
Member

kwonoj commented May 29, 2017

I'm feeling this might be legit case of <T, R=T> with #2614 maybe.

@kwonoj
Copy link
Member

kwonoj commented Jun 2, 2017

And second thought, isn't this PR causes breaking to existing consumers uses publish<T>?

@hermanbanken
Copy link
Contributor Author

@kwonoj the following two types exist:

export function publish<T>(this: Observable<T>): ConnectableObservable<T>;
export function publish<T, R>(this: Observable<T>, selector: selector<T, R>): Observable<R>;

So:

  • users of publish<T>() should not be affected. This will be the largest use case (e.g. publish & connect).
  • users of publish<T>(selector) with a selector that does not change the type of the output would not be affected if they did not explicitly write .publish<T>( in their code. Typescript would automatically infer the new R type from the selector. If they did explicitly write <T> they will need to change that to <T,R>.
  • users of publish<T>(selector) with a selector that does change the type can compile without errors/warnings, only by using my PR. Those people can remove any workarounds (e.g. as any).

@benlesh
Copy link
Member

benlesh commented May 4, 2018

I think this just needs refactored for v6

@cartant
Copy link
Collaborator

cartant commented May 31, 2018

@benlesh To me, it looks like this PR doesn't need to be merged. It adds overload signatures to multicast and to publish for selectors that change the observable's type. However, the current v6 codebase already includes said signatures - they appear to have been added in #2891.

@benlesh
Copy link
Member

benlesh commented May 31, 2018

Ugh! Oh no! Am I closing 2 of @hermanbanken's PRs in one day? 😿

So sorry, @hermanbanken! 🙏

@cartant is right here, this work is already done.

@benlesh benlesh closed this May 31, 2018
@hermanbanken
Copy link
Contributor Author

hermanbanken commented May 31, 2018 via email

@lock lock bot locked as resolved and limited conversation to collaborators Jun 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

TS Issues and PRs related purely to TypeScript issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants