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

Feature request - Ability to access controller or event.params in the registerActionOption function #668

Closed
lb- opened this issue Mar 15, 2023 · 5 comments

Comments

@lb-
Copy link
Contributor

lb- commented Mar 15, 2023

The latest version of Stimulus brought in a powerful way to add custom event listener (action) options.

It would be amazing if the action option function allowed for access to the event.params on the element's action or alternatively the controller instance. Maybe even both of these.

Example usage - current state

For the sake of this example, we want to create a debounce action option that will block events for X ms (default 250) and allows for this value being changed per element. However, there is no easy way to make this value different for actions on the same element.

import { Application } from "@hotwired/stimulus";

const application = Application.start();

application.registerActionOption("debounce", ({ event }) => {
  // this wait value can be read from dataset
  const wait = Number(element.dataset.wait || "250");

  clearTimeout(element._debounceTimer);

  if (event._isDebounced) {
    delete event._isDebounced;
    return true;
  }

  element._debounceTimer = setTimeout(() => {
    event._isDebounced = true;
    element.dispatchEvent(event);
  }, wait);

  return false;
});
<input
  type="text"
  data-controller="filter search"
  data-action="input->filter#apply:debounce keyup->search#query:debounce"
  data-wait="500"
/>

Example usage - event.params

If we had access to the event params (as we do in the method call), this could become easier. We could read event.params and we also get smart value parsing (e.g. string to Number) for 'free'.

import { Application } from "@hotwired/stimulus";

const application = Application.start();

application.registerActionOption("debounce", ({ event }) => {
  // this wait value can be read from event.params and/or dataset
  const wait = event.params.wait || 250;

  // ... same code as above
});
<input
  type="text"
  data-controller="search"
  data-action="input->filter#apply:debounce keyup->search#query:debounce"
  data-filter-wait-param="50"
  data-search-wait-param="750"
/>

Example usage - passing controller

Another approach could be to pass the controller instance to the function, either as this or as an explicit param.

import { Application, Controller } from "@hotwired/stimulus";

class SearchController extends Controller {
  static values = {
    wait: { default: 750, type: Number },
  };
}

class FilterController extends Controller {
  static values = {
    wait: { default: 50, type: Number },
  };
}

const application = Application.start();

// with arg
application.registerActionOption("debounce", handler({ controller, event }) => {
  // the wait value can be accessed from the controller instance
  const wait = controller.waitValue || 250;

  // ... same code as above
});

// using `this`
application.registerActionOption("debounce", function handler({ event }) {
  // this wait value can be accessed from the controller instance
  const wait = this.waitValue || 250;

  // ... same code as above
});
<input
  type="text"
  data-controller="search"
  data-action="input->filter#apply:debounce keyup->search#query:debounce"
  data-filter-wait-value="125"
/>

Implementation

The simplest code change would likely be passing the controller via this or via an argument to the callback but this does feel a bit like the action option has too much power.

It will be a slightly more complex change but still doable to move the addition of the params on the event to a new method that can be called in applyEventModifiers instead of invokeWithEvent. This does feel like it gives a new level of access to the action option without really changing the way to reason about these handlers too much.

passes = passes && filter({ name, value, event, element })

@lb-
Copy link
Contributor Author

lb- commented Mar 15, 2023

I would more than happy to put up a PR for this feature (one or both approaches) if this is considered useful.

@marcoroth
Copy link
Member

I think this addition makes sense 👍🏼

Personally I'd probably want to use the controller approach, but I can also see how passing event.params makes sense, especially if you think about consistency between regular actions and actions with additional action options.

I wouldn't be opposed to see a PR for this, either for one or both approaches.

@lb-
Copy link
Contributor Author

lb- commented Jun 19, 2023

Awesome, I'll give it a go.

lb- pushed a commit to lb-/stimulus that referenced this issue Jun 20, 2023
- Add ability for registerActionOption callbacks to receive the controller instance
- Relates to hotwired#668
lb- pushed a commit to lb-/stimulus that referenced this issue Jun 20, 2023
- Add ability for registerActionOption callbacks to receive the event after `params` have been resolved
- Relates to hotwired#668
@lb-
Copy link
Contributor Author

lb- commented Jun 20, 2023

PRs are up

lb- added a commit to lb-/stimulus that referenced this issue Jun 21, 2023
- Add ability for registerActionOption callbacks to receive the event after `params` have been resolved
- Relates to hotwired#668
lb- added a commit to lb-/stimulus that referenced this issue Jun 21, 2023
- Add ability for registerActionOption callbacks to receive the controller instance
- Relates to hotwired#668
dhh pushed a commit that referenced this issue Jun 26, 2023
…692)

- Add ability for registerActionOption callbacks to receive the event after `params` have been resolved
- Relates to #668
lb- added a commit to lb-/stimulus that referenced this issue Jun 26, 2023
- Add ability for registerActionOption callbacks to receive the controller instance
- Relates to hotwired#668
@marcoroth
Copy link
Member

Resolved via #691 and #692

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants
@lb- @marcoroth and others