Skip to content

Pass change event options from children to parent - #251

Open
valentin-nemcev wants to merge 9 commits into
AmpersandJS:masterfrom
valentin-nemcev:master
Open

Pass change event options from children to parent#251
valentin-nemcev wants to merge 9 commits into
AmpersandJS:masterfrom
valentin-nemcev:master

Conversation

@valentin-nemcev

Copy link
Copy Markdown

I use event options to pass metadata when setting properties, this change fixes a bug when children change event options are not passed to parent event listeners

@dhritzkiv

Copy link
Copy Markdown
Member

This makes sense to me. I'm going to think about this one for a bit.

Could you share some use cases for this? I'm having trouble finding the need to know the options on a change event listener.

Tests seem to pass (despite the saucelabs credentials issue). This would be a semver major change.

@valentin-nemcev

Copy link
Copy Markdown
Author

I called it a bug because I use it for regular set method and totally expected it to work across children-parent boundary, and this behavior is hard to override in client code.

My general use case is selectively skipping event listeners, mostly to break complicated loops in bidirectional bindings. Simplified example is All checkbox:

  • All
  • item 1
  • item 2

All depends on items and items depend on all. In this example you could easily make sure that everything is consistent and no stackoverflow happens when user clicks on checkbox, but sometimes its hard or undesirable because of performance (like when the loop involves a request to backend), or you don't need full consistency. In that case you want to manually break the loop by writing something like this:

// this.allChecked depends on checkbox values
this.listenTo(this, 'change:allChecked',  (model, value, {skipAllCheck} = {}) =>
    skipAllCheck || this.setCheckboxValues(value, {skipAllCheck: true}))

My real use case involved Paginator that is a child of Filters. Filters send their values to backend and backend response updates filter values (including paginator).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants