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

Proposal: change binding #26

Open
Fishrock123 opened this issue Jun 5, 2019 · 8 comments
Open

Proposal: change binding #26

Fishrock123 opened this issue Jun 5, 2019 · 8 comments

Comments

@Fishrock123
Copy link
Owner

Fishrock123 commented Jun 5, 2019

Binding is ugly and I bet it is the least comprehensible part right now.

Ideally I guess it would look something like Stream(source, transform, sink).

@jasnell
Copy link
Collaborator

jasnell commented Jun 6, 2019

Stream(source, transform, sink, options) ?

In JS, options would be an object, in C++ it can be XOR flags.

@Fishrock123
Copy link
Owner Author

Needs an end callback somewhere (or, I guess, it could be a thenable)...

I am not really sure what kind of options you would want to specify at this level... most should be specified when the components are instantiated, imo.

Maybe encoding? I mean, do people actually need to change string encoding mid stream? Does that need to be at this level though?

@jasnell
Copy link
Collaborator

jasnell commented Jun 6, 2019

Thinking about it further, you're right, options are likely not necessary here. I didn't really have any concrete suggestions in mind beyond some basic future proofing, but having it there will mean people will use it and doing so will complicate things more than strictly necessary.

@Fishrock123
Copy link
Owner Author

One big question I ran into yesterday while prototyping is where to put the "final bind callback" that the sink would normally have.

Stream(source, transform, sink, err => {}) // Kinda awkward

Like... I'm not sure this should return a true Promise but maybe it should be thenable? Is that too ludicrous an idea?

@jasnell
Copy link
Collaborator

jasnell commented Jun 6, 2019

For the JavaScript side, I think returning a thenable is completely reasonable.

For the equivalent C++ API, passing in a callbacck function works just fine.

Fishrock123 added a commit that referenced this issue Jun 6, 2019
@Fishrock123
Copy link
Owner Author

Well, this turned out good I think: #31

Fishrock123 added a commit that referenced this issue Jun 10, 2019
Fishrock123 added a commit that referenced this issue Jun 11, 2019
Fishrock123 added a commit that referenced this issue Jun 11, 2019
Introduces `Stream()`, a much more user friendly way of composing
streaming operations out of bob components.

`Stream()`s also act as a bare passthrough to their subcomponents and
can be use as a component when composing higher-order `Stream()`s.

Relys on the newly added `start(exitCb)` to keep things nice.

Refs: #26
PR-URL: #31
@Fishrock123
Copy link
Owner Author

Done in 5a460b4 although we may still want to get rid of bind altogether.

(Or possibly make it so that bindSource does not call bindSink... that would probably improve Stream()...)

@Fishrock123 Fishrock123 reopened this Jun 11, 2019
@Fishrock123 Fishrock123 changed the title Proposal: change binding or make a helper Proposal: change binding Jun 11, 2019
@Fishrock123
Copy link
Owner Author

Fishrock123 commented Jun 11, 2019

Actually, going to keep this open.

Is class-based bind*'s useful? I.e. is there a good reason a class would do some kind of work in there?

If not... this is possible now that the exit callback is done via start():

function bob_bind(source, sink) {
  source.sink = sink
  sink.source = source
}

Edit: oh, right. That still makes implementing Stream() more complex than perhaps necessary. Will keep thinking on it.

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