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

Type empty() with Stream<never> instead of Stream<any> ? #281

Open
ryota-ka opened this issue May 27, 2019 · 2 comments
Open

Type empty() with Stream<never> instead of Stream<any> ? #281

ryota-ka opened this issue May 27, 2019 · 2 comments

Comments

@ryota-ka
Copy link
Contributor

Currently (as of v11.11.0) empty() returns Stream<any>.

xstream/src/index.ts

Lines 1305 to 1310 in 3181720

static empty(): Stream<any> {
return new Stream<any>({
_start(il: InternalListener<any>) { il._c(); },
_stop: noop,
});
}

But IMO Stream<never> is more suitable here.

For example, I ran into the following situation in Cycle.js.

function higherOrderComponent<So extends ..., Si extends ...>(main: Component<So, Si>): ... {
    const { foo = Stream.empty() , ...sinks } = main(sources);
    // `foo` is typed as Stream<any> here
    // Its original type has been lost!
}

If Stream.empty() was typed Stream<never>, foo would retain its original type.

concerns

  • This change requires newer TypeScript version
    • but never type was introduced already in TypeScript v2.0 (almost 3 years ago)
    • Which version of TypeScript does xstream support?
  • This change may introduce some breaking changes to ill-typed codebases.
ryota-ka added a commit to ryota-ka/xstream that referenced this issue Dec 19, 2019
Updated typings so that empty() and never() have Stream<never> type, instead of Stream<any> type.

staltz#281
ryota-ka added a commit to ryota-ka/xstream that referenced this issue Dec 19, 2019
Updated typings so that empty() and never() have Stream<never> type, instead of Stream<any> type.

staltz#281
@staltz
Copy link
Owner

staltz commented Dec 19, 2019

Hey @ryota-ka thanks for this thoughtful issue and the PR too. The reason I didn't answer yet was because this issue is not so obvious what's the right solution.

I'm aware of never, but changing empty() and never() from Stream<any> to Stream<never> has to support the same use cases as before. From the top of my head, here are some important use cases:

  • xs.merge(a$, xs.never()) should be allowed
  • xs.merge(a$, xs.empty()) should be allowed
  • concat(a$, xs.never()) should be allowed
  • concat(a$, xs.empty()) should be allowed
  • Passing xs.never() or xs.empty() to a function f: Stream<T> => B should be allowed, e.g. f(xs.empty())

The problem with Stream<never> is that it doesn't support this last use case. Not in the cases I tested. For instance, if you change xstream's source code according to your PR, and try to build the Cycle.js codebase (the monorepo), it will break in a couple of those cases.

Actually the xs.merge(a$, xs.never()) use case seems well supported by Stream<never> and produces even more meaningful types in the end. But with this change we gain on one side and lose on another side.

@ryota-ka
Copy link
Contributor Author

ryota-ka commented Jan 1, 2020

So are you talking about the variance of the generic parameter T?
Is it related to #286?

BTW, I came up with another typing, does it possibly work...?

- static never(): Stream<any> {
-    return new Stream<any>({ _start: noop, _stop: noop });
+ static never<T = never>(): Stream<T> {
+    return new Stream<T>({ _start: noop, _stop: noop });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants