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

Type signature review progress for v7 beta #5066

Closed
cartant opened this issue Oct 10, 2019 · 9 comments
Closed

Type signature review progress for v7 beta #5066

cartant opened this issue Oct 10, 2019 · 9 comments
Assignees
Labels
TS Issues and PRs related purely to TypeScript issues

Comments

@cartant
Copy link
Collaborator

cartant commented Oct 10, 2019

This issue is for the tracking of the review of the TypeScript signatures - that involve either an Observable or ObservableInput rest parameter or a callback that returns ObservableInput or have open, type-related issues, etc. - as per the discussion in today's core-team meeting.

Deprecations for the scheduler-related changes are not in this list. My understanding is that those deprecations will remain in place, with the breaking changes being made in version 8.

@benlesh As I've review individual source files, I'll check them off in this list if they're okay and will link to a comment (or issue) if changes are required or attention is warranted.

In addition to what's shown below, I think we should look at how null and undefined are used to ensure that passing either- to indicate the absence of an argument - is supported.

@cartant cartant added the TS Issues and PRs related purely to TypeScript issues label Oct 10, 2019
@cartant cartant self-assigned this Oct 10, 2019
@cartant cartant changed the title Type signature review progress Type signature review progress for v7 beta Oct 10, 2019
@cartant
Copy link
Collaborator Author

cartant commented Oct 10, 2019

There is an issue with the Subject typings so that next can be called without an argument for Subject<void>, but the implementation makes Subject unsafe - see #2852. Whether or not anything can be done with this to make things safer, IDK, but we should have another look at it.

@cartant
Copy link
Collaborator Author

cartant commented Oct 10, 2019

IMO, at some stage we should change signatures that involve errors to use unknown instead of any:

export interface NextObserver<T> {
  closed?: boolean;
  next: (value: T) => void;
  error?: (err: unknown) => void;
  complete?: () => void;
}

See #2646

However, this is likely to be an inconvenient breaking change. And it would be reasonable to argue that whilst the TypeScript typings for Promise use any for errors/rejection reasons, this library's typings should continue to use any.

Related Twitter conversation.

@benlesh
Copy link
Member

benlesh commented Jan 20, 2020

startWith and endWith are taken care of here: #5247 #5246

@hmil
Copy link
Contributor

hmil commented Feb 3, 2020

Whether or not anything can be done with this to make things safer, IDK, but we should have another look at it.

Yes, please have another look at it.

The last comment on that issue says it all: You can just replace value?: T by value: T. When T is void, then the argument becomes optional.

const s1 = new Subject<void>();
s1.next(); // OK
s1.next(undefined); // OK

const s2 = new Subject<number>();
s2.next(); // error

The only problem is when someone does this:

const sAny = new Subject(); // No type parameter so defaults to `any`
sAny.next(); // Error because `any` is not mapped to an optional parameter

I don't know what people expect when they create a Subject without an explicit type. The default of Subject<any> is itself dangerous, and could be changed to void:

export declare class Subject<T = void> { /* ... */ }

const sVoid = new Subject(); // Defaults to Subject<void>.
sVoid.next(); // Works fine

If you like to have bugs in your code, then you can still create a subject of any with:

const sAny = new Subject<any>;

@cartant
Copy link
Collaborator Author

cartant commented Feb 15, 2020

OMG, I received a DM question about interop observables and I was writing up some notes that I could turn into an article. The ergonomics of actually using the Subscribable type are horrible. The problem is the number of overloads that are in the type and - when used with from - RxJS always passes an observer - never separate callbacks - so including callbacks in the type is unnecessarily painful.

When #4532 is also taken into consideration, pretty much all aspects of this interop are 👎

@felixfbecker
Copy link
Contributor

@cartant I replied to you in #4532 (comment) :)

@rraziel
Copy link
Contributor

rraziel commented Dec 7, 2020

I will work on the following items' N-args signature/deprecating/renaming this week:

  • operators/concat.ts
  • operators/combineLatest.ts
  • operators/zip.ts

I'll try to open a PR early just so it's visible and there is no overlapping work.

@backbone87
Copy link
Contributor

backbone87 commented Feb 27, 2021

idk, if i should add this here or create a new ticket:

the defaultIfEmpty operator has erroneous & inconvenient typing.

consider the follow example: const a = of(true).pipe(defaultIfEmpty(null));. this results in the following TS error:

Argument of type 'MonoTypeOperatorFunction<null>' is not assignable to parameter of type 'OperatorFunction<boolean, null>'.
  Types of parameters 'source' and 'source' are incompatible.
    Type 'Observable<boolean>' is not assignable to type 'Observable<null>'.
      Type 'boolean' is not assignable to type 'null'.ts(2345)

even explicitly typing a does not work const a: Observable<boolean | null> = of(true).pipe(defaultIfEmpty(null)); (same error as above)

even worse is passing no argument const a: Observable<boolean> = of(true).pipe(defaultIfEmpty());. this produces no error, which is wrong, because the observable should be of boolean | null.

edit: it gets even more worse: const a = of(undefined).pipe(defaultIfEmpty(undefined)); infers a as Observable<undefined> despite being Observable<undefined | null> in reality.

edit2: since this is a bug which should be addressed before ^7 i created a bug ticket: #6064

@cartant
Copy link
Collaborator Author

cartant commented Feb 28, 2021

@backbone87

idk, if i should add this here or create a new ticket

As a general guide - for all repos - if you're unsure, open a new ticket. Anyway, thanks for opening #6064.

@cartant cartant closed this as completed Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TS Issues and PRs related purely to TypeScript issues
Projects
None yet
Development

No branches or pull requests

6 participants