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

Successor for Bacon.fromBinder #704

Open
raimohanska opened this issue Jan 18, 2018 · 9 comments
Open

Successor for Bacon.fromBinder #704

raimohanska opened this issue Jan 18, 2018 · 9 comments

Comments

@raimohanska
Copy link
Contributor

In #703 it was suggested that this version of Bacon.fromEvent should be added:

Bacon.fromEvent(
    (listener) => window.addEventListener('scroll', listener, {passive: true}),
    (listener) => window.removeEventListener('scroll', listener, {passive: true}),
    () => window.scrollX
);

It has the virtue of being more readable than Bacon.fromBinder. From my point of view, it also looks like it should be returning a Property because the third function argument is used to get a current value. The initial value of the Property could be set at the time of creation by calling the third function.

@semmel
Copy link
Member

semmel commented Jan 19, 2018

Ehm isn't that pull request about

Bacon.fromEvent(window, 'scroll', { passive: true }, event => event.prop)

I mean about Bacon.fromEvent and not Bacon.fromBinder?
I find Bacon.fromBinder as general adapter just fine the way it is.

@raimohanska
Copy link
Contributor Author

Ysh

@raimohanska
Copy link
Contributor Author

Oops sorry

@raimohanska raimohanska reopened this Jan 19, 2018
@raimohanska
Copy link
Contributor Author

Yeah there was this suggestion in the comments

@steve-taylor
Copy link

steve-taylor commented Jan 20, 2018

Given the symmetry of bind/unbind calls, how about something like this to avoid duplication:

Bacon.fromEventBinder(
    window,
    (binder, listener) => binder('scroll', listener, {passive: true}),
    () => window.scrollX
);

The second parameter is called with window.addEventListener.bind(window) and, on closing the stream, window.removeEventListener.bind(window).

Edit: This could just be an alternative way of calling Bacon.fromEvent. This would get around some of the future-proofing issues discussed in #703:

  • Bacon.fromEvent(EventTarget|EventEmitter, String [, Function]) (current signature)
  • Bacon.fromEvent(EventTarget|EventEmitter, Function [, Function]) (alternative signature)

The key is the 2nd parameter. If it's a string, Bacon.fromEvent works exactly as it does now. If it's a function, the function is used to bind/unbind the listener to/from the target.

So

Bacon.fromEvent(window, 'scroll')

is equivalent to

Bacon.fromEventBinder(window, (binder, listener) => binder('scroll', listener));

(and eventTransformer can optionally be added to either of these).

@steve-taylor
Copy link

I find Bacon.fromBinder as general adapter just fine the way it is.

Agreed. There will always be use cases for Bacon.fromBinder in its current form that don't involve EventTargets or EventEmitters.

@steve-taylor
Copy link

I'll create a new PR shortly for the following:

Bacon.fromEvent(
    target: EventTarget|EventEmitter,
    eventSource: string|Function,
    eventTransformer?: Function
)

It will be an alternative to #703.

@steve-taylor
Copy link

See #710 and related documentation

@steve-taylor
Copy link

Might as well close this now that #710 is merged.

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

3 participants