fix(publish): fix selector typings

[cartant]

Description:

Fixes the typings for publish and multicast, so that selectors that change the observable's type are supported.

Adds some typings tests for publish and multicast.

Related issue (if exists): #2889

[rxjs-bot]

	Messages
📖

CJS: 1347.9KB, global: 747.2KB (gzipped: 140.5KB), min: 145.3KB (gzipped: 30.9KB)

Generated by 🚫 dangerJS

[cartant]

With the changes in this PR, the selector type exported from operator/publish.ts is no longer used (internally). Should it be removed?

[coveralls]

Coverage remained the same at 97.466% when pulling 2d58972 on cartant:issue-2889 into c7c337e on ReactiveX:master.

[kwonoj]

I recall @benlesh intentionally brought MonoTypeOperatorFunction 🤔

[benlesh]

This should be publish<T, R>(selector?: OperatorFunction<T, R>): OperatorFunction<T, R>

[cartant]

Using OperatorFunction<T, R> instead of any effects this error:

25   return selector ?
            ~~~~~~~~~~
26     multicast(() => new Subject<T>(), selector) :
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
27     multicast(new Subject<T>());
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

dist/src/operators/publish.ts(25,10): error TS2322: Type '((source: Observable<T>) => Observable<R>) | ((source: Observable<T>) => Observable<T>)' is not assignable to type '(source: Observable<T>) => Observable<R>'.
  Type '(source: Observable<T>) => Observable<T>' is not assignable to type '(source: Observable<T>) => Observable<R>'.
    Type 'Observable<T>' is not assignable to type 'Observable<R>'.
      Type 'T' is not assignable to type 'R'.

The problem is the returning of multicast(new Subject<T>()). That call sees the return type inferred as OperatorFunction<T, T> - hence the error.

The problem can be fixed by using any here, instead of in the signature:

  return selector ?
    multicast(() => new Subject<T>(), selector) :
    multicast(new Subject<T>()) as any; // <-- here

Would you prefer that?

[benlesh]

@kwonoj ... this is mostly right. The problem I encountered at work (now that I'm using RxJS with TypeScript all the time, haha) was Observable.of(1, 2, 3).publish(source => source.map(x + '!')) ... would fail because typings didn't permit it. So you had to hack your way through it.

[benlesh]

@cartant if you can try to get this in today I'll try to get it out in the next beta. Because I've had to code around it, and it's annoying. haha

[coveralls]

Coverage remained the same at 97.465% when pulling d91d414 on cartant:issue-2889 into 5dbad94 on ReactiveX:master.

[coveralls]

Coverage remained the same at 97.465% when pulling bc34fff on cartant:issue-2889 into 5dbad94 on ReactiveX:master.

[cartant]

@benlesh After a couple of iterations - each preceded by a cup of coffee - there are now no anys in the typings.

[coveralls]

Coverage remained the same at 97.466% when pulling 2d58972 on cartant:issue-2889 into c7c337e on ReactiveX:master.

[benlesh]

So close

[benlesh]

I think you can go with just OperatorFunction<T, R> as the return type.

[cartant]

Nope. You can't. That will effect the error I mentioned in response to your initial change request.

[cartant]

It's a necessity. I don't see it as a problem, as TypeScript uses only the overload signatures when matching methods calls. It won't match calls against the implementation signature - which is why I have the habit of whacking implementation signatures with the any hammer. That is, the implementation signature does not form part of the method's public contract; it only enforces the internal contract.

[benlesh]

Yup. I see why now... This works. Sorry for any confusion.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.