rxjs: Subject.prototype.next() should only accept T, not T | undefined

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.

About this issue

  • Original URL
  • State: closed
  • Created 7 years ago
  • Reactions: 10
  • Comments: 24 (16 by maintainers)

Commits related to this issue

Most upvoted comments

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.

I think we can solve this with tuple types:

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

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 https://github.com/Microsoft/TypeScript/issues/12400 would make it much easier on us. 😄

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 https://github.com/ReactiveX/rxjs/pull/3074, it sounds like it is still hung up on https://github.com/Microsoft/TypeScript/issues/12400? Or are there other improvements in TypeScript which would allow a fix now?

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