Updates types to eliminate void-emitting channel pitfall

[lourd]

Q	A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?	👍
Tests Added + Pass?	Yes
Any Dependency Changes?

I've run into the pitfall a few times recently of making a void-emitting event channel. I decided that it would be great if the types enforced that incompatibility instead of happily accepting the invalid type, hiding the bug until run time.

[changeset-bot]

🦋 Changeset detected

Latest commit: 9515cc4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages

Name	Type
@redux-saga/core	Minor
@redux-saga/types	Minor
redux-saga	Minor
babel-plugin-redux-saga	Major
@redux-saga/simple-saga-monitor	Major
redux-real-world-example	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Andarist]

I'm unsure about the change because I'm not sure what the mentioned pitfall is. Could you back it up with some examples?

The current types seem to be correct to me as this is a valid situation: TS playground

[lourd]

Exactly, the types allow you to do what will end up with a runtime exception. In your example ch.put(undefined) throws an exception.

[lourd]

Do you want an example of that written in addition to yours and the several tests that are a part of this PR?

[Andarist]

Ah, I forgot about this runtime check:

redux-saga/packages/core/src/internal/channel.js

Line 38 in 2d2214e

check(input, is.notUndef, UNDEFINED_INPUT_ERROR)

Thanks for clarifying! I think that the original intention was there to avoid allowing channel.put() and not necessarily channel.put(undefined) but the runtime code would have to differentiate those cases to perform this runtime validation and that's probably not worth the added complexity.

I've done some testing ({} is always confusing to me in TS) and it looks like everything is alright. Could we add some simple types tests to ensure that both null and other primitives (let's just check a channel with numbers or smth) work OK? I've skimmed through the existing tests and I don't see any like that.

[lourd]

Ok, added tests for covering primitives.

Doing channel.put(null) appears to have an error in TypeScript 3.4 — or at least that popped up when I was running the tests locally. Not sure what to do to resolve that.

[Andarist]

This is an extracted repro case for the mentioned problem with null: TS playground

If we take a closer look at it and hover over channel() call then we might see:

function channel<{}>(buffer?: Buffer<{}> | undefined): Channel<{}>

Of course, this just "propagates" further and we might see a similar result when hovering over .put(null):

(method) Channel<{}>.put(message: {} | END): void

It acts as if null was never part of the constraint. We can even simplify the minal repro case further and repro the same thing with just:

declare function test<T extends {} | null>(): T
test()

My guess is just that behavior in TS around this somehow changed a long time ago - which is interesting, cause this is the exact behavior that we might experience without strictNullChecks (without it we won't have a type error though but null gets "ellided" from the constraint). IIRC strictNullChecks was already a thing back then.

I think that this particular test can just be omitted for those TS@<3.6 tests.

[lourd]

Ok, sounds good.

How do you disable the test? The failure's coming from the non-3.6 directory

Error: redux-saga/packages/core/types/channels.test.ts:77:17
ERROR: 77:17  expect  Compile error in typescript@3.4 but not in typescript@3.5.
Fix with a comment '// Minimum TypeScript Version: 3.5' just under the header.
Argument of type 'null' is not assignable to parameter of type '{} | END'.

    at redux-saga/node_modules/dtslint/bin/index.js:198:19
    at Generator.next (<anonymous>)
    at fulfilled (/Users/louis/code/redux-saga/node_modules/dtslint/bin/index.js:6:58)

I tried putting that minimum version comment at the top of the file but it doesn't seem to have an effect.

[Andarist]

Basically, those files are "sibling" files and for the most part, they are the same (and should be kept the same as much as possible):

packages/core/types/channels.test.ts
packages/core/types/ts3.6/channels.test.ts

The only difference between them is the minimum TS version they are tested against. I didn't fully confirm this but I think the non-ts3.6 files are still tested with TS@>=3.6.

So to remove a "test" you should just remove the discussed assertion from the file in which it has been added. We should add this section of the changes to the packages/core/types/ts3.6/channels.test.ts file though (including the null-related test).

[lourd]

K, done @Andarist

[Andarist]

We just need some changesets here. Remember that you are not constrained to creating a single changeset per PR. I would probably add a single one for @redux-saga/types here and the second one for @redux-saga/core+redux-saga (each describing the change from the perspective of the included packages)