unabridged / motion

Reactive frontend UI components for Rails in pure Ruby
https://github.com/unabridged/motion
MIT License
697 stars 19 forks source link

[Discussion] Debouncing input and the mystery of event targets #42

Closed sosodev closed 4 years ago

sosodev commented 4 years ago

First, I'd like to say that this Gem is awesome. I built my first reactive view component yesterday with little issue. :+1:

Is there an easy way to debounce input events?

For example I have a motion event for search <input class="input" type="text" placeholder="Search" data-motion="input->search">

Which works great but creates a flood of events whenever the user types.

alecdotninja commented 4 years ago

Thanks for the positive feedback, @sosodev ! I'm glad that you were able to get Motion up and running :tada:

Currently, there is not any built-in way to debounce motions; however, if you're willing to write some JavaScript, the client does expose the ability to trigger motions programatically via motion.getComponent(element).processMotion(motion, event). Here is how that might look with Stimulus:

<input class="input" type="text" placeholder="Search" data-controller="example" data-action="input->search">
// app/javascript/controllers/example_controller.js
import { Controller } from "stimulus"
import motion from "../motion"

export default class extends Controller {
  search(event) {
    if (this.timeout) {
      clearTimeout(this.timeout)
    }

    this.timeout = setTimeout(() => {
      this.timeout = null

      motion.getComponent(event.target).processMotion("search", event)
    }, 1000)
  }
}

This isn't really an answer to your question, but it is worth noting that if you used the change event instead of input, the motion would only be triggered once when the input looses focus instead of on each key press.

sosodev commented 4 years ago

Thanks for the info. :) Manually triggering the event seems like a good solution. I tend to avoid change events though since I think the idea of a focused input doesn't make any sense to the average user.

I'll go ahead and close this as solved. Hope you have a great weekend.

sosodev commented 4 years ago

Hi @alecdotninja, I found a slight problem while implementing this. Currently Motion attempts to serialize the currentTarget for the event but currentTarget is null by the time the debounced query fires and it causes an error.

Is there any particular reason why Motion chooses to provide both the target and currentTarget? I'm testing a naive patch that serializes the target as the currentTarget and it seems to work fine without requiring the API to change at all. Alternatively, allowing currentTarget to be null would work too but might be a little confusing for new users who expect that value to always be set on the server side.

alecdotninja commented 4 years ago

Good catch, @sosodev . Thank you for being the trailblazer on this. :smile:

The reason that we send currentTarget (and indeed bless it as the default choice by aliasing it to Motion::Event#element) is that this is the element to which the user added the data-motion attribute, and it is likely where they have placed any other data attributes they want to use as "arguments" to their motion.

When writing that code, I didn't realize that currentTarget could be null (I thought it was always equivalent to this inside of the event handler :sweat_smile:).

I would gladly accept a patch to gracefully allow currentTarget to be null. That said, I think the ideal solution is actually to ditch currentTarget completely, start sending the element to which the motion was bound, and expose that as Motion::Event#element.

sosodev commented 4 years ago

Oh, I see what you mean. Determining the actual target element is a bit tricky. :sweat_smile:

Maybe processMotion could be modified to accept a motionElement parameter, for custom stuff like debouncing, and query the component itself for the element when the motionElement is null.

I'm imagining it looking something like this...

// motion/javascript/Component.js

  processMotion (name, event = null, motionElement = null) {
    if (!motionElement) {
      motionElement = this.element.querySelector(`[data-motion=${name}]`)
    }

    if (!this._subscription) {
      this.client.log('Dropped motion', name, 'on', this.element)
      return false
    }

    this.client.log('Processing motion', name, 'on', this.element)

    const extraDataForEvent = event && this.client.getExtraDataForEvent(event)

    this._subscription.perform(
      'process_motion',
      {
        name,
        event: event && serializeEvent(event, extraDataForEvent, motionElement)
      }
    )

    return true
  }

Modifying serializeEvent to emit our intended element like you mentioned

// motion/javascript/serializeElement.js

export default function serializeEvent (event, extraData = null, element = null) {
  const { type } = event
  const details = serializeEventDetails(event)
  const target = serializeElement(event.target)

  return {
    type,
    details,
    extraData,
    target,
    element: element && serializeElement(element)
  }
};

and simply doing away with current_target and the alias

# motion/lib/motion/event.rb

    def element
        return @element if defined?(@element)

        @element = Motion::Element.from_raw(raw["element"])
    end

This does seem like a cleaner solution. :smile: What do you think?

alecdotninja commented 4 years ago

Sorry for the delay, @sosodev :see_no_evil: .

I like where this is going, but can we change the default in the case the element is not passed to something like this:

event.currentTarget || event.target.closest(`[data-motion=${name}]`)

I actually think the event.target.closest approach will always give the right answer since (AFAIK) events only bubble up the DOM.

alecdotninja commented 4 years ago

In the next release of Motion, processMotion will accept an optional 3rd argument for the Motion::Event#element. :tada: That said, my thinking on this issue has changed.

I think the ideal way to solve this problem is with something like debounced rather than hand-rolling the debouncing JavaScript. It turns out this approach works with or without #43.

alecdotninja commented 4 years ago

I'm going to go ahead and close this for now. Please feel free to reopen, @sosodev, if you have any questions or think something more should be done here. :smile: