kefirjs / kefir

A Reactive Programming library for JavaScript
https://kefirjs.github.io/kefir/
MIT License
1.87k stars 97 forks source link

Support Event Listener Options #247

Closed jcbelanger closed 7 years ago

jcbelanger commented 7 years ago

It would be nice if Kefir.fromEvents supported passing event listener options to the addEventListener call underneath. For example, you may need to pass the passive flag to for touch performance reasons or want to attach to the capture phase with useCapture. Example:

document.addEventListener('touchstart', handler, {passive: true});
jackjennings commented 7 years ago

I also implemented passive listeners on a project recently. I'm not sure that this needs to be added to Kefir proper, but I ended up resorting to copy/pasting some internal Kefir code into my project in order to accomplish this:

import { stream } from "kefir";

// Passive listener check from https://github.com/WICG/EventListenerOptions/blob/gh-pages/explainer.md
// Test via a getter in the options object to see if the passive property is accessed
var supportsPassive = false;

try {
  var opts = Object.defineProperty({}, "passive", {
    get: function() {
      supportsPassive = true;
    }
  });
  window.addEventListener("test", null, opts);
} catch (e) {}

// https://github.com/rpominov/kefir/blob/b4a69587729bf9f17b6bfaf5b7252f4e4211a8c8/src/utils/functions.js#L30
function apply(fn, c, a) {
  let aLength = a ? a.length : 0
  if (c == null) {
    switch (aLength) {
      case 0:
        return fn()
      case 1:
        return fn(a[0])
      case 2:
        return fn(a[0], a[1])
      case 3:
        return fn(a[0], a[1], a[2])
      case 4:
        return fn(a[0], a[1], a[2], a[3])
      default:
        return fn.apply(null, a)
    }
  } else {
    switch (aLength) {
      case 0:
        return fn.call(c)
      default:
        return fn.apply(c, a)
    }
  }
}

// https://github.com/rpominov/kefir/blob/b4a69587729bf9f17b6bfaf5b7252f4e4211a8c8/src/primary/from-sub-unsub.js#L4
function fromSubUnsub(sub, unsub, transformer /* Function | falsey */) {
  return stream(function(emitter) {
    let handler = transformer
      ? function() {
          emitter.emit(apply(transformer, this, arguments))
        }
      : x => {
          emitter.emit(x)
        }

    sub(handler)
    return () => unsub(handler)
  }).setName('fromSubUnsub')
}

function fromEventsPassive(target, eventName, transformer) {
  const options = supportsPassive ? { passive: true } : false;

  return fromSubUnsub(
    handler => target.addEventListener(eventName, handler, options),
    handler => target.removeEventListener(eventName, handler, options),
    transformer
  ).setName("fromEventsPassive");
}

Perhaps fromSubUnsub could be a candidate for exposing in the public API, in order to allow for this kind of functionality?

jcbelanger commented 7 years ago

I believe adding the options parameter to Kefir.fromEvents is a good thing because it will give developers the same power the DOM api exposes to addEventListener/removeEventListener.

I think the change is very minimal and can be implemented without breaking changes. I've referenced this issue in a recent pull request for reference. Here's how example usage would look like:

var scrollStream = Kefir.fromEvents(div, 'scroll'); //same behavior
var passiveScrollStream = Kefir.fromEvents(div, 'scroll', null, { passive: true });

I don't think we should create a new Kefir.fromEventsPassive because it would the same problem. How do I customize options for those events? Users may want to pass useCapture ,once, and future values that browsers add support for together with the passive option! With that said, I believe it should be the caller's responsibility to check for support and perform the proper checks in. Maybe the argument can be made for ending a streams if user passes the once flag in the options parameter, but that's another topic and easy for users to implement with this change in place.

Additionally, the code @jackjennings posted demonstrates further why this change should be part of Kefir. Not only did he have to duplicate apply/fromEvents/fromSubUnsub (which is a useful utility and I also wish was exposed publicly), but his implementation only supports addEventListener/removeEventListener variants and not the addListener/removeListener and on/off pairs that Kefir.fromEvents does. People's re-implementations of Kefir.fromEvents will not be as efficient or carry the same support.

jackjennings commented 7 years ago

As a library consumer, I agree—it could be nice to be able to pass the newish options into addEventListener, and my workaround is extremely verbose for what it does.

However, I can see from the perspective of a library maintainer that a) adding the feature detection for passive events in the library directly is a hard pill to swallow (while not including it means answering support questions unrelated to the core library), and b) the current fromEvents interface doesn't support adding an additional object of options (passing null as an argument is fairly ugly).

As an interim solution, I'd be happy to have access to fromSubUnsub if only to write my own passive event listener library that conforms to my preferences. It seems like the least invasive change to the library and would help make variations of the fromEvents function easier to produce.

jcbelanger commented 7 years ago

I agree that passing null to Kefir.fromEvents is ugly. I would love to make the api match the DOM api 1 to 1, but backwards compatibility means supporting transformer. (Personally, I don't think transformer parameter should be part of any of the APIs we already have Kefir.map and it ruins future expansions like this). I agree, that providing access to fromSubUnsub would be good enough.

mAAdhaTTah commented 7 years ago

Part of the issue with this proposal is Kefir.fromEvents isn't for DOM events only, but also works with on / off and addListener / removeListener pairs, neither of which necessarily support this. That said, Kefir.fromEvents isn't particularly magical. You could easily just do:

Kefir.stream(emitter => {
  target.addEventListener('click', emitter.value, { passive: true });
  return () => target.removeEventListener('click', emitter.value, { passive: true });
});
rpominov commented 7 years ago

I agree with @mAAdhaTTah . I'd also just use Kefir.stream() in this case. I want Kefir API to be simple, because simple API is easier to learn. Also code that uses a simple API is easier to understand. Little complications like this add up.

jackjennings commented 7 years ago

Ah, nice—I just made my version by looking through the source, and didn't consider other options.

Also, I've found transformer to be helpful when the callback passes more than one argument and I need to process them into a single value, fwiw.

jcbelanger commented 7 years ago

I think we all agree that Kefir.stream() is the way to go. Thanks!