fix(Subject): make value parameter for next non-optional

[felixfbecker]

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 #2852

[rxjs-bot]

	Messages
📖

CJS: 1377.9KB, global: 747.4KB (gzipped: 119.7KB), min: 145.2KB (gzipped: 31.3KB)

Generated by 🚫 dangerJS

[coveralls]

Coverage remained the same at 97.078% when pulling 64ba901 on felixfbecker:next-not-optional into cd9626a on ReactiveX:master.

[benlesh]

Given that I've personally written a lot of code that calls Subject next() without an argument, I'm not sure how I feel about this one. @david-driscoll ... is there some way to have the best of both worlds here? Like a no-argument override that only exists for Subject<void>?

cc @kwonoj

[kwonoj]

afaik there isn't great way. Thought of defeult generic bit, but neither <T=void> nor <T=never> doesn't work - default generic seems only works when value is actually supplied.

[benlesh]

I'm still thinking about this one. Consulting some TypeScript folks for a second opinion. FWIW: I'm totally on the fence about this change.

[Airblader]

I can't say whether this is intended, but

declare function next<T extends void>(): void;
declare function next<T>(item: T): void;

next<void>(); // accepted
next<void>(1); // rejected

next<number>(); // rejected
next<number>(1); // accepted

seems to work. However, it does also allow

next<void>(undefined);
next<void>(null);

where at least the latter is probably not ideal, but perhaps a good compromise.

[kwonoj]

@Airblader Subject value type rely on class parameteric generic, I don't think we'll going to have function level generic param separately. For class generics, we can't have overload.

[Airblader]

Fair enough. For what it's worth, I'm not a fan of this change. void subjects are a frequent usecase and writing undefined explicitly looks and feels odd there. And that's on top of being a breaking change which will break a lot of codebases (admittedly the fix is simple).

Having a non-generc VoidSubject type would be an option, but a separate type seems a bit extreme I guess.

[kwonoj]

I've been thinking this bit, while I like explicitly of interface, there are still lot of cases of using next() as pure signal for subject.

I'd like to just suggest to make signature to next(value: T = undefined) instead - while it doesn't change any actual user behavior (yes, unfortunatly even VS code lsp will still remain as value?: T) but it'll cleary document in our type definition for intended behavior at least.

[felixfbecker]

Maybe we should open an issue at TypeScript to treat undefined and "not supplied" as equivalent (that's the JS runtime behaviour after all).

But for me, I would always choose type safety over not having to type undefined. If you don't care about type safety and don't want to write extra code, use JavaScript.

[kwonoj]

But for me, I would always choose type safety over not having to type undefined. If you don't care about type safety and don't want to write extra code, use JavaScript.

Well, I do also prefer type safety. So say when we decided to have default param next(value = undefined) is it really not providing type safety at all? It is not clear explicit around param must be specified, but at least gives loosened indication of default value will be undefined. I don't think it's losing complete type safety in here. Especially, in here actually we do treat next() as valid cases in general, just we didn't express in typescript definitions.

[felixfbecker]

With value = undefined we would have the exact problem as we have right now.

[kwonoj]

I am saying next() is allowed, while indicate it'll have default value as undefined while current signature doesn't indicate it. Main difference is by specifying default value, now runtime explicitly assign undefined as default value when it's not specified. So in here with my proposal, subject.next() // Should not be allowed! is different with you - I'm saying we may just subject.next() // Should be allowed, and you'll force assign undefined as default value.

[kwonoj]

@benlesh thoughts? I honestly bit indifferent around this matter, claritywise either would just work. One for breaking, one for not (and bit loosend as user still don't get enforce to supply param).

[felixfbecker]

My point is there is zero difference from a type safety perspective

[kwonoj]

I think that's part I disagree.

[felixfbecker]

Current state: No compile error when sending in undefined into a typed Subject<string>
With your proposal: No compile error when sending in undefined into a typed Subject<string>

Where does the type safety come in? The runtime JS is slightly different, but that obviously doesn't make it more type safe. The signature is slightly different, which could be seen as part of "documentation". But nothing will actually prevent you from writing/committing/merging/deploying that bug.

[kwonoj]

actually prevent you from writing/committing/merging/deploying that bug

Your statement is based on interface asks next must deliver any value when T is defined. If we follow that statement, you are correct there isn't type safety around. What we currently (implies) are we do actually allow opposite cases, where possibly changed signature correctly indicate that. It is indeed it doesn't carry over T for those cases, but I don't think that makes completely losing type safety. It narrows down to how we really define interface / behavior of next().

As I commented in original issue, I am generally in favor of strong types with explicit clarity, but also it is somewhat widely used pattern of using subject as signal. If we have TSC support like this:

class Subject<T> {
    next(item: T): void {

    }
}

const voidSubject = new Subject<void>();
voidSubject .next() // T is void, so allowed

const normalSubject  = new Subject<string>();
normalSubject .next() // Should not be allowed!

then I would be all for those, but unfortunately that doesn't seem to work with class based parametric generic but only work with function overload.

[kwonoj]

But again, as said I'm bit indifferent in this matter (as myself strongly prefer explicit type, while my suggestion is workaround for widely-used pattern for subject). I'm trying to advocate those usecases with workaround proposals.

[felixfbecker]

Then you would have to change the .subscribe() signature too so that the observer doesn't receive T but always T | undefined, which would be a nightmare with strictNullChecks on for Subjects where T is not void. Otherwise you do lose type safety. subscribe() can currently be called with an Observer that expects string if T is string, but it is possible it may receive undefined if next() is called without a parameter. That is a type safety failure.

[david-driscoll]

Rock meet hard place.

On the one hand, allowing implicit undefined values in for next() can cause issues for consumers that forgot to put a value in. On the other hand there are very valid cases for using next() this way strictly as a signal (and other scenarios I'm sure).

I can't really lean either way. The strongly typed developer in me wants to supply the value, the JavaScript developer in me wants to allow people to use the foot gun if they forget to pass a value.

[kwonoj]

yeah, pretty much same @david-driscoll (guess reason we are having long threads here. 😅 )

[kwonoj]

Oh wow, actually digging bit more and found very similar proposal here: microsoft/TypeScript#12400 (comment)

(copy-pasta here)

In a perfect world, we would like:

let a: Promise<number>;
a.resolve(); // not allowed
a.resolve(4); // ok

let b: Promise<void>;
b.resolve(); // ok
b.resolve(4); // not ok

let c: Promise<number|undefined>;
c.resolve() // not ok
c.resolve(4) // ok
c.resolve(undefined) // ok

and this is accepted proposal, makes me thinking rather wait compiler side support.

[kwonoj]

Ok, I'm closing this PR itself for now, and let's track this with upstream changes. When proposed change comes in, we can satisfy all conditions without breaking changes.

[benlesh]

It seems like TypeScript 2.8 conditional generics may solve this well according to @jasonaden. I'll hold off for that, because I'm not fond of this change, although I definitely understand the spirit of the change.

[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.