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

Subject.prototype.next() should only accept T, not T | undefined #2852

Closed
felixfbecker opened this issue Sep 23, 2017 · 24 comments · Fixed by #7290
Closed

Subject.prototype.next() should only accept T, not T | undefined #2852

felixfbecker opened this issue Sep 23, 2017 · 24 comments · Fixed by #7290
Labels
TS Issues and PRs related purely to TypeScript issues

Comments

@felixfbecker
Copy link
Contributor

RxJS version: 5.4.3

Code to reproduce:

const subject = new Subject<string>()

subject.next()

Expected behavior:

Compile error

Actual behavior:

Compiles

Additional information:

next()'s argument is typed as optional, which means it allows undefined even if the Subject's type does not. It should only allow T, not T | undefined.

@kwonoj
Copy link
Member

kwonoj commented Sep 23, 2017

This.. seems makes sense to me? I think it's just kind of legacy when we deal with old version of TSC have limitation with union types.

@kwonoj kwonoj added the TS Issues and PRs related purely to TypeScript issues label Sep 23, 2017
@kwonoj
Copy link
Member

kwonoj commented Sep 23, 2017

/cc @david-driscoll

@felixfbecker
Copy link
Contributor Author

Could we get this in before 6.0 as it's a slight breaking change?

@kwonoj
Copy link
Member

kwonoj commented Oct 6, 2017

It's breaking anyway, isn't it?

@felixfbecker
Copy link
Contributor Author

What's breaking anyway?

@kwonoj
Copy link
Member

kwonoj commented Oct 6, 2017

Someone accidentally supplied undefined by not noticing it until. We had similar cases before, as long as it possibly breaking user code then we simply consider it as breaking even though it's obviously our side issue.

Honestly I'm not very tempted to touch type definition in v5 currently, lot of them are written with old versions of TSC compiler and we need revise huge breaking changes with v6 anyway, also changing v5 signature with old version surprisingly can affect user code in latest TSC. (not saying this one's those case)

@felixfbecker
Copy link
Contributor Author

Yeah, that's why I'm asking whether we could get this change to be in 6.0

@kwonoj
Copy link
Member

kwonoj commented Oct 6, 2017

Oh, I thought you're asking it to be in 5.x. my bad. This should be in 6 in any cases.

@kwonoj
Copy link
Member

kwonoj commented Oct 6, 2017

Just clarifying lot of v6 issues are backlogged, cause next branch is not usable state so I'm not even accepting PR at this moment. We should release 5.5 and realign 6 branch asap.

felixfbecker added a commit to felixfbecker/rxjs that referenced this issue Nov 14, 2017
Prevents emitting undefined on strictly-typed Subjects

BREAKING CHANGE:
If you are using TypeScript called next() without a value before, TypeScript will error. If the Subject is of type void, explicitely pass undefined to next().

closes ReactiveX#2852
@benlesh
Copy link
Member

benlesh commented Nov 15, 2017

So, to be clear to others, if someone wants to use it as: subject.next() they would need to create a new Subject<void>().

@felixfbecker
Copy link
Contributor Author

@benlesh not entirely sure whether that would work and might depend on your compilerOptions. At least in #3074 I had to explicitly call subject.next(undefined).

@benlesh
Copy link
Member

benlesh commented Nov 15, 2017

Hmmm... this is nasty. This effectively means with TS you would never be able to call next() on Subject, it looks like. :\ I'm not sure this is ideal. There are plenty of situations were you just want to notify and not pass a value. It's ugly that you'd be forced to pass undefined.

@felixfbecker
Copy link
Contributor Author

felixfbecker commented Nov 15, 2017

I agree it is not nice, even though I personally would still prefer that over the risk of someone calling it without a value and therfor emitting undefined somewhere into code that doesn't expect it.

I tried to overload next but I can't make a type constraint on the class T parameter to only apply the overload if T is void

I think this is the TypeScript issue that would make this possible: microsoft/TypeScript#12400
Maybe we can express this issue there

@Dynalon
Copy link

Dynalon commented Aug 21, 2018

The current situation in v6.x is still very unexpected to me as a TypeScript developer when using the --strict tsc compiler option (or the --strictNullChecks option to be specific):

const someSubject = new Subject<string[]>();

// will throw an error when s is undefined!
// Worse: The type of arr is inferred as string[], but it
// actually should have been (string[] | undefined)
someSubject.subscribe(arr => arr.forEach(s => console.info(s));

// this line should be a compile error in --strict mode
someSubject.next();

rxjs Subjects will use the generic type argument and make it nullable: string[] | undefined in this case which is somewhat counterintuitive. If I wanted to allow undefined, I would have defined the type as Subject<string[] | undefined>. These explicit defines are common and wanted if I turn on strictNullChecks.

The fact that I always have to pass undefined when I create a Subject<void> is not surprising, but standard when using TypeScript in strict mode and should be known to the developer:

function print<T>(t: T) {
    console.info(t);
}
// this is a compiler error
print<void>();
// this is ok
print<void>(undefined);

I don't think rxjs should do some "magic" conversion of T to T | undefined as me as a developer would not expect this. I ran into a bug where I had a .next() in the code, and the subscriber function had a default argument initializer - so it worked at first. In a refactor I removed the default argument in the subscriber function and the tsc compiler did not report an error, but turned into a runtime error. The whole idea of strictNullChecks is to avoid this.

Even worse is that the typing of the subscribe() function is wrong: It is infered as T (and displayed as T in the IntelliSense) but should actually be T | undefined - the tsc compiler will then enforce the non-null/non-undefined checks inside the subscriber function and we are back to type safety:

const someSubject = new Subject<string[]>();

someSubject.subscribe(arr => {
    // assuming arr will be inferred to (string[] | undefined) then
    // tsc will error here, saying that arr is possibly 'undefined'
    arr.forEach(s => console.info(s)
});

Please reconsider changing this. This change affects only users which enable strict nullchecks, that option is off by default and when off still allows me to call .next() without passing undefined.

@wulfsberg
Copy link

I find it very problematic that the type signature of the observable does not match up with what you're allowed to .next into it. That is a null-pointer crash waiting to happen.
But tracking it through #3074, it sounds like it is still hung up on microsoft/TypeScript#12400? Or are there other improvements in TypeScript which would allow a fix now?

@david-driscoll
Copy link
Member

It's entirely possible that this could be supported via some sort conditional types, I haven't investigated it yet. I'll see if I can this afternoon (I have a free hour or two which is unusual).

However the missing type from microsoft/TypeScript#12400 would make it much easier on us. 😄

@david-driscoll
Copy link
Member

Sadly this cannot be done, I hit a hard wall where conditional types are not allows to be used on anything by a type and they cannot be used as a target for implements.

I was able to model out the types just fine, however once you implement both the VoidObserver and ValueObserver<T> interfaces things fall apart because once you create a new Subject<void> the type system sees two methods one for VoidObserver and one for ValueObserver<T> where you actually only want one implementation.

We could maybe work around this by having a factory function to create subjects that returns a subject that implements the correct Observer interface but that feels like a huge cludge and wouldn't be easily discoverable by users.

export interface NextVoidObserver {
  closed?: boolean;
  next: () => void;
  error?: (err: any) => void;
  complete?: () => void;
}

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

export interface ErrorVoidObserver {
  closed?: boolean;
  next?: () => void;
  error: (err: any) => void;
  complete?: () => void;
}

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

export interface CompletionVoidObserver {
  closed?: boolean;
  next?: () => void;
  error?: (err: any) => void;
  complete: () => void;
}

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

export type PartialVoidObserver = NextVoidObserver | ErrorVoidObserver | CompletionVoidObserver;
export type PartialValueObserver<T> = NextValueObserver<T> | ErrorValueObserver<T> | CompletionValueObserver<T>;
export type PartialObserver<T> = PartialVoidObserver | PartialValueObserver<T>;

export interface VoidObserver {
  closed?: boolean;
  next: () => void;
  error: (err: any) => void;
  complete: () => void;
}

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

export type Observer<T> = T extends void ? VoidObserver : ValueObserver<T>;

However you should be able to cast (or assign) a subject to one of these interfaces as the declaration should match either or, but you would have to add the interface to your code or a shared library you control.

@wulfsberg
Copy link

Sounds like holding out for microsoft/TypeScript#12400 is still the best option, then. Thanks for the attempt.

@mina-skunk
Copy link

This is huge! I just found this out. We have huge codebase and now we have to question undefined checking every single Observable with a Subject source and every angular output ( EventEmitter extends Subject ).

@felixfbecker
Copy link
Contributor Author

I think we can solve this with tuple types:

interface Subject<T> {
  next(...args: T extends void ? [] | [T] : [T]): void
}

@cartant
Copy link
Collaborator

cartant commented May 30, 2019

@felixfbecker @benlesh

I think we can solve this with tuple types

Nice. That looks like a good solution, to me.

@hediet
Copy link

hediet commented Jun 27, 2019

you can also do this:

interface Subject<T> {
   next(this: Subject<void>): void;
   next(arg: T): void;
}

But anyways, typescript already consideres void parameters as optional.
Thus,

interface Subject<T> {
  next(...args: T extends void ? [] | [T] : [T]): void
}

is equivalent to

interface Subject<T> {
  next(arg: T): void;
}

@cartant
Copy link
Collaborator

cartant commented Mar 14, 2021

Closing this because in v7 Subject has a next that looks like this:

next(value: T) {
this._throwIfClosed();
if (!this.isStopped) {
const copy = this.observers.slice();
for (const observer of copy) {
observer.next(value);
}
}
}

@cartant cartant closed this as completed Mar 14, 2021
@raveclassic
Copy link

raveclassic commented Jan 23, 2023

Hey folks, looks like Subscriber class has the same issue:

new Observable<string>(subscriber => {
  subscriber.next() // should throw in compile-time!
})

cc @cartant

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
10 participants