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
- fix(Subject): make value parameter for next non-optional Prevents emitting undefined on strictly-typed Subjects BREAKING CHANGE: If you are using TypeScript called next() without a value before, Typ... — committed to felixfbecker/rxjs by felixfbecker 7 years ago
- fix(Subscriber): next method no longer has optional value argument resolves #2852 — committed to benlesh/rxjs by benlesh a year ago
- fix(Subscriber): next method no longer has optional value argument resolves #2852 — committed to benlesh/rxjs by benlesh a year ago
- fix(Subscriber): next method no longer has optional value argument (#7290) resolves #2852 — committed to ReactiveX/rxjs by benlesh a year ago
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):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 allowundefined
, I would have defined the type asSubject<string[] | undefined>
. These explicit defines are common and wanted if I turn onstrictNullChecks
.The fact that I always have to pass
undefined
when I create aSubject<void>
is not surprising, but standard when using TypeScript instrict
mode and should be known to the developer:I don’t think rxjs should do some “magic” conversion of
T
toT | 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 ofstrictNullChecks
is to avoid this.Even worse is that the typing of the
subscribe()
function is wrong: It is infered asT
(and displayed as T in the IntelliSense) but should actually beT | undefined
- the tsc compiler will then enforce the non-null/non-undefined checks inside the subscriber function and we are back to type safety: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
.Closing this because in v7
Subject
has anext
that looks like this:https://github.com/ReactiveX/rxjs/blob/4805a7fecfa5d9732e0329fb463e11d62c219238/src/internal/Subject.ts#L54-L62
I think we can solve this with tuple types:
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 anew Subject<void>()
.