Pass data attributes as action method argument #249

adrienpoly · 2019-05-03T16:47:33Z

Very often the question comes up on how to pass arguments to Stimulus action methods.

Global parameters can be passed to the action using the data API but when a controller has multiple elements and individual parameters (an id by example) for each of those elements, it is a common practice to pass the value using the standard dataset API: event.currentTarget.dataset.id

One typical example would be for a set of buttons with ids and associated fetch actions

<div data-controller="items" data-items-base-url="https://api.stimulus.org/upvote/">
  <button data-action="items#upvote" data-id='1'></button>
  <button data-action="items#upvote" data-id='2'></button>
  <button data-action="items#upvote" data-id='3'></button>
</div>

...
upvote(e) {
  const id = e.currentTarget.dataset.id
  fetch(this.baseUrl + id, { method: "POST" })
}

get baseUrl() {
  return this.data.get("baseUrl")
}
...

Proposal

This PR is a draft proposal for a more concise API by passing all data attributes value of the currentTarget as a parameter of the action.

action(event, data) {}

Using basic destructuring the above example can be simplified like this:

...
upvote(e, { id }) {
  fetch(this.baseUrl + id, { method: "POST" })
}
...

I think this could make it simpler to understand and avoids confusions around target and currentTarget.
If the element is of a type HTMLFormElement we could also pass the value at the same time.

Just wanted to propose this for now if you feel this could make sense, I ll add some associated test

tpetry · 2019-07-10T09:56:38Z

Wouldn't passing a dataset object like this.data of controllers fit much more to the stimulus way?
If there are multiple actions for an element the data values may interfere. I think it would be best to prefix them with the controller and action, so data-id would be data-items-click-upvote-id.

@jaan @sstephenson Whats your opinion on the pull request? It's small but a very usable improvement

dhh · 2019-07-10T10:53:17Z

I'd like to see a complete params concept encapsulate this. So:

<div data-controller="items" data-items-base-url="https://api.stimulus.org/upvote/">
  <button data-action="items#upvote" data-items-upvote-id-param='1'></button>
  <button data-action="items#upvote" data-items-upvote-id-param='2'></button>
  <button data-action="items#upvote" data-items-upvote-id-param='3'></button>
</div>

// Call signature is event, params
upvote(e, { id }) {
  fetch(this.baseUrl + id, { method: "POST" })
}

This way params are separated from any other type of data-* on the action element, they're namespaced so different actions can have different params, and we have a concept that's encapsulated fully.

dhh · 2019-07-10T11:02:45Z

Actually, I'd rather still even just make the event part of the params, so you can chose whether or not to destruct it:

// Doesn't need the event
upvote({ id }) {
  fetch(this.baseUrl + id, { method: "POST" })
}

// Does need it event
upvote({ id, event }) {
  fetch(this.baseUrl + id, { method: "POST" })
  event.preventDefault()
}

tpetry · 2019-07-10T11:04:49Z

This would break backwards compatibility

adrienpoly · 2019-07-10T12:39:20Z

I'd like to see a complete params concept encapsulate this. So:
<div data-controller="items" data-items-base-url="https://api.stimulus.org/upvote/">
  <button data-action="items#upvote" data-items-upvote-id-param='1'></button>
  <button data-action="items#upvote" data-items-upvote-id-param='2'></button>
  <button data-action="items#upvote" data-items-upvote-id-param='3'></button>
</div>
This way params are separated from any other type of data-* on the action element, they're namespaced so different actions can have different params, and we have a concept that's encapsulated fully.

I started implementing and testing with this namespacing approach. I agree that it does fully encapsulate the params but it could become very verbose to write if we have several controllers needing the same parameter

This same example with the id become very long to write.

 <div data-controller="controller1 controller2" data-items-base-url="https://api.stimulus.org/upvote/">
   <button data-action="controller1#upvote controller2#doSomething" 
                 data-controller1-upvote-id-param='1' 
                 data-controller2-upvote-id-param='1'></button>
   <button data-action="controller1#upvote controller2#doSomething" 
                  data-controller1-upvote-id-param='2' 
                  data-controller2-upvote-id-param='2'></button>
 </div>

The js part is simplified but the html becomes much more complex to write.

I think that the type of parameters that we need to pass to the functions are unique as they are linked to the caller element of the action. That was my idea behind this for not using a namespace.

I like the suggestion of adding the param prefix. I think it would make it easier to port the value api #202 to this feature with something like:

export default class extends Controller {
  static params = {
    id: Number,
    active: Boolean
  }
 ...

  upvote(e, { id, active }) {
    //active is a Boolean
    if (active) { 
      fetch(this.baseUrl + id, { method: "POST" })
    }
  }
}

dhh · 2019-07-10T13:01:50Z

Don't have a problem bumping a major version for this, and we can have a feature toggle that still supports the old signature.

dhh · 2019-07-10T13:44:10Z

@javan Has an idea for this that doesn't involve breaking backwards compatibility. I'll let him present that.

javan · 2019-07-10T13:58:18Z

I'm not a fan of moving event into an object because it makes the most common usage like calling event.preventDefault() more verbose. You'd have to either destructure the event out or access a nested params.event property. And it's breaking change.

Another option, which wouldn't be a breaking a change, is to hang params off the event object:

<button data-action="items#upvote" data-items-upvote-url-param="/items/1/votes">…</button>

upvote(event) {
  event.preventDefault()
  console.log(event.params) // { url: "/items/1/votes" }
}

Or, if you're only interested in the params:

upvote({ params }) {
  console.log(params) // { url: "/items/1/votes" }
}

Or even:

upvote({ params: { url } }) {
  console.log(url) // "/items/1/votes"
}

dhh · 2019-07-10T14:02:34Z

I also do like the idea of defining the params ala values, btw. Rather than having them be fully namespaced in the attributes. Would give us the power to typecast as well.

The only issue is that it's a global params definition, even if the invocation is mostly per-action. But maybe that doesn't matter. A lot of the time there's bound to be overlap anyway (like with ids).

javan · 2019-07-10T14:58:51Z

Because params would be read-only (unlike values which are read/write), we could (attempt to) typecast them automatically without the need to declare types in the controller. Example:

function getParams(element, descriptor) {
  const params = {}
  const [ identifier, action ] = descriptor.split("#")
  const pattern = new RegExp(`^data-${identifier}-${action}-(.+)-param$`)
  for (const { name, value } of element.attributes) {
    const match = name.match(pattern)
    const key = match && match[1]
    if (key) params[key] = typecast(value)
  }
  return params
}

function typecast(value) {
  try {
    return JSON.parse(value)
  } catch (o_O) {
    return value
  }
}

<button data-action="item#upvote" 
  data-item-upvote-id-param="12345" 
  data-item-upvote-url-param="/votes" 
  data-item-upvote-active-param="true">…</button>

❭ getParams(button, "item#upvote")
{ id: 12345, url: "/votes", active: true }

dhh · 2019-07-15T08:40:36Z

I like that. The primary appeal for declaring the params up front would be a sense that you'd say "this controller uses these params", especially since you'd feel more OK with having the params listed without controller/action namespaces. But starting to to think that it probably doesn't matter. We could even do both.

So the default would be that params are simply available to all actions, but you could also namespace them, in case there was a conflict between two equally named params for the same element. So:

<button data-action="item#upvote" 
  data-id-param="12345" 
  data-item-upvote-url-param="/votes" 
  data-item-upvote-active-param="true">…</button>

In that case, params.id would be available to all controllers/actions invoked off that element. But params.url and params.active would only be there for the item#upvote. I think this works particularly well since params is now just a event.params call, that you can destruct as you please on the method signature.

saiqulhaq · 2019-09-08T06:39:47Z

please merge this PR
currently not easy to pass argument

I think what @dhh suggested is very good

jmas · 2019-09-22T09:10:41Z

@saiqulhaq what about:

<button type="button" data-action="controller.doAction" data-id="1">Do Action</button>

controller:

doAction(event) {
    console.log('data attached to button=', event.target.dataset);
    console.log('id=', event.target.dataset.id);
}

or:

doAction({ target: { dataset: { id } } }) {
    console.log('id=', id);
}

saiqulhaq · 2019-09-23T04:13:21Z

@jmas data-id is not very clear for me
if we use data-id-param, then it means it's a parameter for stimulus's action.
and also we can add params property to event object

so it could be

doAction({ params }) {
    console.log('id=', params.id);
}

adrienpoly · 2019-09-23T07:10:33Z

@saiqulhaq I haven't had time yet to update this PR as per our talk above. I plan to do so by the end of the month.

chrislabarge · 2019-11-28T06:05:44Z

I love the idea of global and controller/action specific params. This would allow me to open the scope of a lot of my stimulus controllers.

rubab · 2020-05-21T08:06:56Z

How can I access data params in controller action?

saiqulhaq · 2020-05-22T07:08:45Z

@rubab you can use $(event.target).data(' data attribute here ')

scarroll32 · 2020-05-31T11:17:54Z

This looks great @adrienpoly will it be merged?

jturmel · 2020-07-29T15:23:11Z

Is this PR still in the works?

adrienpoly · 2020-07-29T15:34:49Z

@jturmel yeah still need to work on it. Last time I hit a few limits of my TS knowledge but I have practiced a bit more TS since then. I ll try to get back to it in August but if you feel like taking over feel free to give it a try

dancallaghan · 2020-10-11T22:24:44Z

I see a possible issue with having shared params. What constitutes a shared param vs. a namespaced param? Would it be a case of looking through each of the actions, finding their namespaced params and the remainder are shared?

In this example:

 <button data-action="item#upvote remote#load"
  data-id-param="12345"
  data-remote-load-url-param="/votes/summary"
  data-item-upvote-url-param="/votes">…</button>

Would remoteLoadUrl be passed to item#upvote? Or would it be excluded because it belongs to another action's namespace?

Maybe we could get around the ambiguity by using a JSON syntax? Seems a little messier though…

<button data-action="remote#load item#upvote"
  data-params="{ id: 12345 }"
  data-remote-load-params="{ url: '/votes/summary' }"
  data-item-upvote-params="{ url: '/votes', active: true }">…</button>

Another option would be to have a reserved global param namespace. Something like shared/global

 <button data-action="item#upvote remote#load"
  data-shared-id-param="12345"
  data-remote-load-url-param="/votes/summary"
  data-item-upvote-url-param="/votes">…</button>

adrienpoly · 2020-12-07T14:10:33Z

I have noticed a few things that needs to be corrected after the merge with 2.0 will try to address them

adrienpoly · 2020-12-10T20:38:58Z

I have corrected a few merge issues with 2.0 and updated a bit the code to match as close as possible to the latest additions.

The features remain the same as described in my previous post

As always let me know if you have questions / suggestions / comments

sagarrishabh · 2021-02-25T13:12:20Z

Merge this bro, I need it. 😄

leehericks · 2021-03-29T16:32:51Z

@adrienpoly thank you for your hard work!

I’ve been thinking about the API and documentation for this PR all day.

The targets, values, and css classes APIs all have clear, explicit declarations at the top of the controller.

Actions methods currently

are left empty upvote()
or reference the event object upvote(event)

The params API should similarly have clear declarations in the action method arguments.

I think the rest of the event object can still be accessed with this destructuring syntax:

upvote({ params: { id, url = “/test/me” }, ...event })

but this buries the event at the end of the arguments.

I’d like to suggest adding an event getter property to the event object (in addition to params):

get event() {
  return this;
}

which allows us the write the event at the front of the destructured list:

upvote({ event, params: {
  id = undefined, 
  url = undefined, 
  active = true } }) {
  ...
}

allowing the params to feel like an appended optional declaration:

upvote()
upvote(event)
upvote({ event, params: { id, url = “/fallback/path” } })

As a side note, inserting params into the event object still feels very awkward to me. Does this not feel cleaner?

upvote(event, params) {
  const {
    id = undefined, 
    url = undefined, 
    active = true 
  } = params
  ...
}

dancallaghan · 2021-03-29T21:46:50Z

I feel like destructuring too much in the function declaration reduces readability (agreeing with your side note). Why not something like this?

upvote(event) {
  const {
    id = undefined, 
    url = undefined, 
    active = true 
  } = event.params
  // ...
}

leehericks · 2021-03-29T23:15:07Z

@dancallaghan Indeed, this is much clearer, but it still irks me that the params are riding in the event object and we can’t require developers to declare params like we do for targets, values and css classes. Then again, technically the developers could just access the data bypassing those other APIs, so the final solution needs to offer enough benefit that everyone wants to use it.

Are we naming this feature correctly if they don’t appear in the method parameters? Are we not really just referring to a subset of data values declared on the element with the action descriptor?

leehericks · 2021-03-30T00:17:48Z

Then again, we can simply add a section to the end of Event Objects in the Actions reference entitled “Event Params”, where we can show the best practice example of declaring the event params.

so, this covers getters and @javan said here that these params are read-only so no need for setters.

I don’t remember seeing discussion above about adding existential properties, which could be a useful check that lets the developer know, for example, that the attribute wasn’t in the data (or is undefined?) so the restructured value is the provided default value.

If we go with the “Event Params” branding then what about event.hasIdParam which checks if the param is defined in the data tags?

While we may not want to overly pollute the event object, event.params.hasIdParam feels redundant.

Also event.params.hasId does not match with the targets, values and css classes APIs.

seb-jean · 2021-09-22T15:19:04Z

Hi,
I tried this new feature and I have always undefined value when I do :

upVote({params}) {
    console.log(params)
}

adrienpoly · 2021-09-22T16:46:46Z

@seb-jean are you running the hotwired/stimulus 3 beta 2 version ?

panoply · 2022-03-20T02:48:42Z

Same issue as @seb-jean in v3.0.1 using the @hotwired/stimulus module. Params are simply not passed.

Looking at the source for the actions logic, the re-assignment of the event target is a bit of code smell imo and just to pass the params object? Also, why is the params logic wrapped in a getter, deconstructed and finally re-assigned? Just feels so extraneous, I get the approach but it's definitely not ideal.

adrienpoly force-pushed the data-attributes branch from ff86e24 to cbe6b48 Compare November 26, 2020 18:09

pass data attributes object as a method parameter

61f804e

adrienpoly force-pushed the data-attributes branch from 0b580d5 to 61f804e Compare November 27, 2020 05:36

adrienpoly added 5 commits November 27, 2020 12:58

adds deepEqual to test log controller

d3b7344

pass data-xx-param as action paramters

b5d9cf3

typecast the values

af2f564

multi word params are camelized

ec2c5f1

adds controller namespace

c5c22f4

adrienpoly added 5 commits December 4, 2020 14:59

Merge remote-tracking branch 'upstream/master' into data-attributes

99118ab

Merge remote-tracking branch 'upstream/master' into data-attributes

f634a2a

rename ExtendedEvent to ActionEvent

ad80917

use built in camelize function

e24e89f

Ensure eventTarget is an Element

0ef12f8

adrienpoly added 4 commits December 7, 2020 15:48

move action params tests to the correct directory

c9d7106

custom assertAction with deepEqual

d4fa042

fix: empty string params should return an empty string

ef15aa6

add missing new line EoF

b766c31

adrienpoly mentioned this pull request Dec 10, 2020

add Meta suffix to meta getters stimulus-use/stimulus-use#84

Merged

adrienpoly added 2 commits December 10, 2020 20:40

Refine params type for ActionLogEntry

e1992c4

split action params getters with a more expressive function name

a078d21

Base automatically changed from master to main January 12, 2021 20:57

leehericks mentioned this pull request Mar 30, 2021

Custom default values for the Values API #350

Merged

adrienpoly force-pushed the data-attributes branch from b276290 to 310927c Compare June 14, 2021 17:28

correct the typing of ActionEvent

6b40e2b

adrienpoly force-pushed the data-attributes branch from 310927c to 6b40e2b Compare June 14, 2021 17:30

Merge branch 'main' into data-attributes

696d82f

dhh merged commit 0d81949 into hotwired:main Jul 1, 2021

adrienpoly deleted the data-attributes branch July 1, 2021 07:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass data attributes as action method argument #249

Pass data attributes as action method argument #249

adrienpoly commented May 3, 2019

tpetry commented Jul 10, 2019

dhh commented Jul 10, 2019 •

edited

Loading

dhh commented Jul 10, 2019 •

edited

Loading

tpetry commented Jul 10, 2019

adrienpoly commented Jul 10, 2019 •

edited

Loading

dhh commented Jul 10, 2019

dhh commented Jul 10, 2019

javan commented Jul 10, 2019

dhh commented Jul 10, 2019

javan commented Jul 10, 2019

dhh commented Jul 15, 2019

saiqulhaq commented Sep 8, 2019

jmas commented Sep 22, 2019 •

edited

Loading

saiqulhaq commented Sep 23, 2019

adrienpoly commented Sep 23, 2019

chrislabarge commented Nov 28, 2019

rubab commented May 21, 2020

saiqulhaq commented May 22, 2020

scarroll32 commented May 31, 2020

jturmel commented Jul 29, 2020

adrienpoly commented Jul 29, 2020

dancallaghan commented Oct 11, 2020

adrienpoly commented Dec 7, 2020

adrienpoly commented Dec 10, 2020

sagarrishabh commented Feb 25, 2021

leehericks commented Mar 29, 2021

dancallaghan commented Mar 29, 2021

leehericks commented Mar 29, 2021

leehericks commented Mar 30, 2021

seb-jean commented Sep 22, 2021

adrienpoly commented Sep 22, 2021

panoply commented Mar 20, 2022

Pass data attributes as action method argument #249

Pass data attributes as action method argument #249

Conversation

adrienpoly commented May 3, 2019

Proposal

tpetry commented Jul 10, 2019

dhh commented Jul 10, 2019 • edited Loading

dhh commented Jul 10, 2019 • edited Loading

tpetry commented Jul 10, 2019

adrienpoly commented Jul 10, 2019 • edited Loading

dhh commented Jul 10, 2019

dhh commented Jul 10, 2019

javan commented Jul 10, 2019

dhh commented Jul 10, 2019

javan commented Jul 10, 2019

dhh commented Jul 15, 2019

saiqulhaq commented Sep 8, 2019

jmas commented Sep 22, 2019 • edited Loading

saiqulhaq commented Sep 23, 2019

adrienpoly commented Sep 23, 2019

chrislabarge commented Nov 28, 2019

rubab commented May 21, 2020

saiqulhaq commented May 22, 2020

scarroll32 commented May 31, 2020

jturmel commented Jul 29, 2020

adrienpoly commented Jul 29, 2020

dancallaghan commented Oct 11, 2020

adrienpoly commented Dec 7, 2020

adrienpoly commented Dec 10, 2020

sagarrishabh commented Feb 25, 2021

leehericks commented Mar 29, 2021

dancallaghan commented Mar 29, 2021

leehericks commented Mar 29, 2021

leehericks commented Mar 30, 2021

seb-jean commented Sep 22, 2021

adrienpoly commented Sep 22, 2021

panoply commented Mar 20, 2022

dhh commented Jul 10, 2019 •

edited

Loading

dhh commented Jul 10, 2019 •

edited

Loading

adrienpoly commented Jul 10, 2019 •

edited

Loading

jmas commented Sep 22, 2019 •

edited

Loading