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

Shouldn't ResponseStream extend Stream? #547

Open
mightyiam opened this issue Mar 10, 2017 · 21 comments
Open

Shouldn't ResponseStream extend Stream? #547

mightyiam opened this issue Mar 10, 2017 · 21 comments

Comments

@mightyiam
Copy link

Here is ResponseStream.

Since it is a stream, perhaps it should extend xstream's Stream?

I'm trying to provide a ResponseStream to xstream's combine and I'm getting this:

Argument of type 'ResponseStream' is not assignable to parameter of type 'Stream<any>'. Property '_prod' is missing in type 'ResponseStream'.
@mightyiam
Copy link
Author

Actually, perhaps I should use Stream<Response>, instead? So, I'm not sure this is a bug. Other than it might be confusing that ResponseStream is exported.

But the whole notion of exported interface is yet not clear to me. How do I know which ones are public. I mean, I do import them just like JavaScript values. Is a change to an exported interface considered breaking? Are these exported interfaces usually documented? I just look at the source of Cycle.js, snabbdom and xstream and import from there. I try to be reasonable about what I use but it seems pretty much dictated by TypeScript. If I desire type checking then I must use those types.

@TylorS
Copy link
Member

TylorS commented Mar 10, 2017

Yeah, I believe it should be ResponseStream extends Stream<Response> {}

@wclr
Copy link
Contributor

wclr commented Mar 11, 2017

Maybe ResponseStream is not very good naming here.

@mightyiam
Copy link
Author

@whitecolor I don't get it. Why not? That's pretty much what it is, isn't it? Did you see my PR? You're welcome to review it.

@wclr
Copy link
Contributor

wclr commented Mar 12, 2017

I don't know what is was meant by @staltz under ResponseStream, maybe should reviewed/reconsidered and made consistent to avoid such word ambiguity.

@staltz
Copy link
Member

staltz commented Mar 13, 2017

I think the only problem here is the naming, which can be confusing. But we actually need the separate interface, and we cannot do ResponseStream extends Stream<Response> because most-typings.d.ts and rxjs-typings.d.ts use it as select(category?: string): Observable<Observable<Response> & ResponseStream>.

@staltz
Copy link
Member

staltz commented Mar 13, 2017

Actually, perhaps I should use Stream<Response>, instead?

There isn't the field request on stream.request, so we need Stream<Response> & ResponseStream so we can get both xstream APIs and .request.

How do I know which ones are public.

The exported ones are public.

I mean, I do import them just like JavaScript values.

They are TypeScript types, not JavaScript values (e.g. they never show up in the compiled JS)
You don't actually need to import the interface at all because the HTTPSource will have those types in its methods.

Is a change to an exported interface considered breaking?

Usually it is.

@mightyiam
Copy link
Author

Well, then. The naming.

It shouldn't be called ResponseStream because it is not a Stream. Proving that it is not a stream is that when you use it, you must ResponseStream & Stream.

So, what it is, seems to me like what I did in #548. Which is RequestBearing. It states the actual meaning of it. It bears a request. Now, I don't have enough TypeScript or any strongly typed language experience to know whether RequestBearing follows good conventions. Yet, it seems to me that the intention is correct.

@staltz
Copy link
Member

staltz commented Mar 16, 2017

I know the naming could be better, but in what cases would you require importing ResponseStream? One idea I have is to simply embed the type alias without a name:

select(category?: string): Stream<MemoryStream<Response> & {request: RequestOptions}>;

Because HTTPSource.select is pretty much the only public API that has this. But I don't exclude the idea of making RequestBearing or something similar. I would probably convert it from interface to type alias, though, so that it becomes a required field, and not a "named interface". TypeScript details...

@mightyiam
Copy link
Author

The bottom line for me is to be able to import the type that is emitted by the stream that select returns.

For the purpose of

const fooResponse$$: Stream<[WhatHereNow]> = HTTP.select('foo')

@staltz
Copy link
Member

staltz commented Mar 27, 2017

Usually I write just

const fooResponse$$ = HTTP.select('foo')

Because the type of fooResponse$$ is inferred automatically by TypeScript. But would it be so bad to write this? (notice that {request: RequestOptions} is just 11 characters longer than RequestBearing)

type Res$$ = Stream<MemoryStream<Response> & {request: RequestOptions}>;
const fooResponse$$: Res$$ = HTTP.select('foo')

Notice that even if we have RequestBearing, it would be quite verbose:

const fooResponse$$: Stream<MemoryStream<Response> & RequestBearing> = HTTP.select('foo')

or even

type Res$$ = Stream<MemoryStream<Response> & RequestBearing>;
const fooResponse$$: Res$$ = HTTP.select('foo')

That's why I recommend

const fooResponse$$ = HTTP.select('foo')

@mightyiam
Copy link
Author

While my journey with TypeScript is yet short, I find it useful to declare types that I assume would otherwise be inferred.

Declaration > inference. Because inferred type mighty be not what one thinks and also for documentation that could not be wrong (unlike comments).

That's why I gave it a name and made it exported.

@3n-mb
Copy link

3n-mb commented May 28, 2017

Response is something that comes for a given request.
Stream of pushed events and response to a request, initiated by you, are two fundamentally different things.
@staltz , I think this is an echo of pull-n-push question. Changing consumer's pull into push, i.e. ResponseStream, again shows to be confusing ( #289 ).
And this is an async pull we have here, not just a sync pull, which, as you said in #581 has also been bugging people.
In other words, Erik is indeed around the corner. We do need pull 🙏

@3n-mb
Copy link

3n-mb commented May 28, 2017

@mightyiam I don't think that you should be belittling your understanding of TS. Not at all. Typing simply highlights an existing cycle problem here, which is trying to turn consumer's pull semantic into consumer's push. Erik talks overall about this in a very nice talk (thanks to @staltz for link, talk in enlightening).

@wclr
Copy link
Contributor

wclr commented May 29, 2017

@3n-mb

This question was discussed many times, about is there is a need in a pull in HTTP like drivers, it seems like it breaks cyclic semantic and basically approach. When you are in starbucks and you order coffee on one side of the counter, you expect to get your cup on the other side of the counter with a named label attached when coffee is ready, is it a pull, are you pulling your cup of coffee or just waiting it pushed?

@3n-mb
Copy link

3n-mb commented May 29, 2017

@whitecolor

  1. Despite <quote>This question was discussed many times<\quote> people hit it. Sometimes they hit it in non-direct form, like here, developer has a feeling that something is not smooth with a type. I simply want to point to a true cause of this: lack of pull, for which HTTP is candidate number 1, but for those of us consuming pull apis from electron platform, pull isn't only about HTTP. If on a simple page a hacky HTTP approach, of turning consumer's pull into consumer's push, can be swallowed, on a bigger thing, there will be lo-o-ots of places begging for exposing pull api. Multiplying hack to many other places is not advisable, to put it lightly.

  2. Cycle, as we articulated in Push-Pull proposal: Signals to complement Streams #581, has the following future-directed advantage. Push based flow makes it so that control and data flows are the same, making them visualizable. Hacks like turning consumer's pull into consumer's push mess up data-flow graph, removing its intuition-helping power. Exploratory PR in that same issue shows that we can have a structure that is consumed as pull, while is implemented as producer's push. Your express app is essentially pushed by a request. What if this service component lives locally, in your electron app? (Reminding again that pull is not just about HTTP).

  3. Note how easy it is to be arguing for adding a pull. Just wait for another confused developer to show up 😈 . In fact, one needed to wait till the core team itself hit a need for pull (Push-Pull proposal: Signals to complement Streams #581), even sync.

  4. Now, the Starbucks thing. If I don't like were they serve coffee, I don't go there. 😄
    Yet, I know that cycle has a goldmine in it (visual stuff). It just misses an official recognition that pull is ok, where it is needed. Is this a symptom of silver bullet flu, so easily contracted by new promising projects?
    You see, I am not worried about myself, not finding a counter, at which I get the coffee. I am worried about my team. All these issues, that are in essence a need for pull, are going to surface and eat our time. And this is not ok.
    You see, I am trying to sell cycle to my team, to actual consumers of a framework. They give feedback. I bring this feedback to cyclejs issues. Devs are pleading 🙏 for mercy. Turning consumer's pull into consumer's push is a pain.

@3n-mb
Copy link

3n-mb commented May 29, 2017

@whitecolor
This question <quote>are you pulling your cup of coffee or just waiting it pushed</quote> is answered as follows. Business logic actually waits. The whole process, which I want to visually see, waits.
Invoking relativity of time perception, is a cute argument, but even in physics, one has to settle into a reference frame to make practical calculations.
If business logic needs a pull, it must be a pull. (Pull consists of push of producer + wait for result.)

@wclr
Copy link
Contributor

wclr commented May 29, 2017

You see, I am trying to sell cycle to my team, to actual consumers of a framework. They give feedback. I bring this feedback to cyclejs issues. Devs are pleading 🙏 for mercy. Turning consumer's pull into consumer's push is a pain.

What is the problem, add pull to HTTP driver, or what you use a see how it works for you. I use my own drivers set, and HTTP like drivers are based on the same driver called task and it has pull method exeperimentally

@3n-mb
Copy link

3n-mb commented May 29, 2017

@whitecolor
First, it is http driver. Then it is file saving driver. Etc. Then I need a pre-and-post processing around pull. Following logic, pre/post processing should be dropped into new driver that envelopes existing driver (I want clean concerns separation and code reuse).
But code placed into driver is not visible/debuggable with cycle tool. Cycle's selling point is this visualization.

In fact, I don't want to write drivers. I want to write nicely flowing circuits that may have some pull-ing nodes. In a visual tool, clicking on pull node can open a circuit responsible for producing result to a pull. Such producer circuit will be like other cycle circuits, and it will be visualizable in all tools.
Pull nodes become a tiny bit of glue that turns customer pull into producer push. That's it.
It has also been noted in #581, that turning of pull into push is an essence of driver. Adapting pull-helping nodes we may reduce number of drivers we write, and increase number of cycle circuits!

@3n-mb
Copy link

3n-mb commented May 29, 2017

Wow, @whitecolor , I see you actually created repo that allows you to have async pulls. Isn't this is one more point that pull should be added to cycle?
Take a look, at #581, please. Critic it, add to it, etc. We can solve this in a best way! ❤️
Disclamer, @staltz opened #581 aming only at sync pull case, but my fear is that he will miss a bigger pull opportunity due to narrowing on a tiny element, while wasting time by breaking existing streams, which doesn't seem to be necessary.

@wclr
Copy link
Contributor

wclr commented May 29, 2017

but my fear is that he will miss a bigger pull opportunity due to narrowing on a tiny element while wasting time by breaking existing streams, which doesn't seem to be necessary.

It is his own time and he probably knows what he's doing, and you is not restricted in any way to use cycle framework as you like to Just don't depend on this so eagerly, no need to fear. Cycle is really simple, you even may use you own run core or stream lib, if you really don't like something "core team" is doing =)

I had my comments in #581

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

No branches or pull requests

5 participants