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.

Sign up for GitHub

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

Jump to bottom

Events with more than one argument #84

Closed
jfedyczak opened this issue Jul 18, 2016 · 10 comments · Fixed by #89
Closed

Events with more than one argument #84

jfedyczak opened this issue Jul 18, 2016 · 10 comments · Fixed by #89
Labels
help wanted issue is suggestion weight 2 (could)

Comments

@jfedyczak
Copy link

As per suggestion of @xtianjohns I'm creating new issue conected with this issue. There's a problem with emitting events with more than one argument as in:

var fromEvent = require('xstream/extra/fromEvent').default
var http = require('http')

var srv = http.createServer()

fromEvent(srv, 'request')
.addListener({
  next: function(req, res) { // <-- problem here
      // res is undefined
      res.end(req.url)
  },
  error: function(err) { console.error(err) },
  complete: function() { console.log('completed') },
})

srv.listen(3002)

Possible solution would be emitting an array with all the args or passing all the args...
@TylorS
Copy link
Collaborator

TylorS commented Jul 18, 2016

@jfedyczak I don't think Server's are really a use case for xstream, as it's designed for (and only for) Cycle.js

@jfedyczak
Copy link
Author

jfedyczak commented Jul 18, 2016 •
edited
Loading

@TylorS Well, it is just an example of use with EventEmmiter event which is now supported by xstream as an extra... It seems unnatural to use another library for server side. Also I see it as a fix for somewhat incomplete implementation of fromEvent for EventEmitter events :)

@staltz
Copy link
Owner

staltz commented Jul 19, 2016

xstream listener functions, just like ES Observable or RxJS, will always take just one argument. If there is an API that deviates from that, then we need to convert multiple args to an array or an object that collects them. I'd be happy to get a PR for this for fromEvent.

@sculeb
Copy link

sculeb commented Jul 19, 2016 •
edited
Loading

It would help if the fromEvent had selector support like rxjs. Then one could map multiple arguments of the event to an object or just use the argument he needs or w/e.

usecase:

// mqtt-connection
client.subscribe(topic);
fromEvent(client, 'message', (topic, message) => message.toString())
.addListener({
next: val => console.log(val),
error: err => console.log(err),
complete: () => console.log('done')
});

@xtianjohns
Copy link
Contributor

xtianjohns commented Jul 19, 2016 •
edited
Loading

I'm excited to take a stab at supporting these. @jfedyczak, I worked on the fromEvent support for Event Emitters, and you're right that the current implementation lacks the same features as some other stream libraries. One concern was that supporting selectors using the same signature as some other libraries would be a breaking change. Specifically, the third argument (used for DOM event support) would need to be moved.

I can get a PR in for this, but if folks have concerns about a potentially breaking change for the extra and want that to be tackled in a particular way, let's discuss. My instincts are to move the (currently) third argument into an optional fourth argument, which would be a hash of options.

Does this seem reasonable?

// current
function fromEvent(element: EventTarget | EventEmitter,
                   eventName: string,
                   useCapture?: boolean = false): Stream<Event|any> {}
// new
// `EventOptions`, `SelectorFn` interfaces are glossed over here
function fromEvent(element: EventTarget | EventEmitter,
                   eventName: string,
                   selector?: SelectorFn,
                   options?: EventOptions): Stream<Event|any> {}

edited to include an example of the updated signature

@staltz
Copy link
Owner

staltz commented Jul 20, 2016

I find selector?: SelectorFn, options?: EventOptions problematic since it gives special treatment to the selector, and it makes the total count of parameters be 4.
How about putting the selector inside the options?

@xtianjohns
Copy link
Contributor

Yeah, four parameters is a lot. I don't really like the method-on-options approach, but I can't express why. It just feels fishy to me.

So, that signature (in my example) is what Rx implements. But most.js's arguments are the same as xstream's current implementation. However, it aggregates all of the arguments into an array passed to the listener, only if there are multiple arguments. This might be harder for us to implement in the extra, but it'd enforce that stream modifications take place in map(), which is probably where they should belong.

Which way are folks leaning?

@staltz
Copy link
Owner

staltz commented Jul 20, 2016

But most.js's arguments are the same as xstream's current implementation. However, it aggregates all of the arguments into an array passed to the listener, only if there are multiple arguments. This might be harder for us to implement in the extra, but it'd enforce that stream modifications take place in map(), which is probably where they should belong.

most.js's way seems the best in my opinion. I don't think it'd be that hard to implement.

@xtianjohns
Copy link
Contributor

Cool - I'll take that approach then. Thanks!

xtianjohns pushed a commit to xtianjohns/xstream that referenced this issue Jul 22, 2016
feat(fromEvent): Aggregate multiple arguments
92c2aad 
If provided with an EventEmitter, `fromEvent` streams will emit an array
of values when the EventEmitter provides listeners with multiple
arguments.

The previous implementation naively provided only the first argument,
which made `fromEvent` incompatible with event interfaces that rely on
providing multiple arguments to their listeners, such as Node's
`http.Server`.

An alternative solution involved providing selector support. This wasn't
implemented for several reasons.

  1. Modifying the function signature for the extra would be a
     breaking change.
  2. An expanded function signature would become 'crowded', and one of
     the architectural goals of xstream is to keep interfaces as small
     as they need to be to achieve their goals.
  3. Aggregating emitted events into an array is consistent with the
     implementation in Most.js, and achieves similar flexibility with Rx
     when providing a selector function to `.map()` on a result stream.

`fromEvent` will now emit mixed-types. If consumers are not responsible
for calling `.emit()` on the source emitter, they should implement
appropriate guards to ensure they are dealing with an intended type.

Should resolve staltz#84.
@xtianjohns xtianjohns mentioned this issue Jul 22, 2016
Merged
5 tasks
xtianjohns pushed a commit to xtianjohns/xstream that referenced this issue Jul 22, 2016
feat(fromEvent): Aggregate multiple arguments
e36fd48 
If provided with an EventEmitter, `fromEvent` streams will emit an array
of values when the EventEmitter provides listeners with multiple
arguments.

The previous implementation naively provided only the first argument,
which made `fromEvent` incompatible with event interfaces that rely on
providing multiple arguments to their listeners, such as Node's
`http.Server`.

An alternative solution involved providing selector support. This wasn't
implemented for several reasons.

  1. Modifying the function signature for the extra would be a
     breaking change.
  2. An expanded function signature would become 'crowded', and one of
     the architectural goals of xstream is to keep interfaces as small
     as they need to be to achieve their goals.
  3. Aggregating emitted events into an array is consistent with the
     implementation in Most.js, and achieves similar flexibility with Rx
     when providing a selector function to `.map()` on a result stream.

`fromEvent` will now emit mixed-types. If consumers are not responsible
for calling `.emit()` on the source emitter, they should implement
appropriate guards to ensure they are dealing with an intended type.

Should resolve staltz#84.
staltz pushed a commit that referenced this issue Jul 22, 2016
@staltz
feat(fromEvent): Aggregate multiple arguments
714dd01 
If provided with an EventEmitter, `fromEvent` streams will emit an array
of values when the EventEmitter provides listeners with multiple
arguments.

The previous implementation naively provided only the first argument,
which made `fromEvent` incompatible with event interfaces that rely on
providing multiple arguments to their listeners, such as Node's
`http.Server`.

An alternative solution involved providing selector support. This wasn't
implemented for several reasons.

  1. Modifying the function signature for the extra would be a
     breaking change.
  2. An expanded function signature would become 'crowded', and one of
     the architectural goals of xstream is to keep interfaces as small
     as they need to be to achieve their goals.
  3. Aggregating emitted events into an array is consistent with the
     implementation in Most.js, and achieves similar flexibility with Rx
     when providing a selector function to `.map()` on a result stream.

`fromEvent` will now emit mixed-types. If consumers are not responsible
for calling `.emit()` on the source emitter, they should implement
appropriate guards to ensure they are dealing with an intended type.

Should resolve #84. Was PR #89.
@staltz
Copy link
Owner

staltz commented Jul 22, 2016

Done in v5.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted issue is suggestion weight 2 (could)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(fromEvent): Aggregate multiple arguments xtianjohns/xstream
5 participants
@staltz @jfedyczak @xtianjohns @sculeb @TylorS