Add support of space-delimited list of events #3010
Conversation
docs/marionette.functions.md
Outdated
| @@ -173,6 +173,9 @@ All arguments that are passed to the triggerMethod call are passed along to both | |||
|
|
|||
| `triggerMethod("foo", bar)` will call `onFoo: function(bar){...})` | |||
|
|
|||
| To trigger several events and corresponding methods split the events by the space. Example: | |||
| * `triggerMethod("render ready")` fires the "onRender" ans the "onReady" functions | |||
There was a problem hiding this comment.
actually can we use triggerMethod('foo bar')? Triggering render manually is probably not a great idea.
|
"render" event was chosen just as an example. I've changed the event names to more abstract. |
|
@rise2semi this is great work! Could you please write a test for events bubbling up the view tree? to prove that childEvents can listen for events triggered like this. |
|
childViewEvents 😂 |
|
Is there any perf concerns here? This is a pretty hot function |
|
On the map / trigger multiple part I dont see any perf downside, at least no more than manually triggering multiple events at once from the view. Should not be visible. My concern is more about the 1st test, if it costs a little, it's a little x all the time |
# Conflicts: # test/unit/trigger-method.spec.js
|
@paulfalgout I've added tests for childViewEvents. I'm not sure that there will be performance issue with |
|
Should we look to resolve these conflicts and bring this feature in for 3.1 or (potentially) 3.2? |
| export function triggerMethod(events, ...args) { | ||
| var result; | ||
|
|
||
| if (eventSplitter.test(events)) { |
There was a problem hiding this comment.
do we need this if statement? "foo".split(/\s+/) returns an array
| // event and call the "onFooBar" method and then trigger the "baz:qux" event | ||
| // and call the "onBazQux" method | ||
| export function triggerMethod(events, ...args) { | ||
| var result; |
| @@ -7,20 +7,17 @@ import getOption from './utils/getOption'; | |||
| // split the event name on the ":" | |||
| const splitter = /(^|:)(\w)/gi; | |||
|
|
|||
| // split the list of events on the space | |||
| var eventSplitter = /\s+/; | |||
|
@scott-w I am not a big fan of this implementation. triggerMethod is my 2nd perf woe and I am sure this implementation is going to create a hit in the perf. Let me think about how to help this PR move forward a bit. |
|
Fair enough @rafde, I definitely see a lot of |
|
@rise2semi this branch is out of sync at the moment. If you are still interested in helping, please rebase and resolve conflicts. |
|
closing as it is stale |
Add support of space-delimited list of events for triggerMethod.
The problem is described in #2937.