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

onBeforeElUpdated for events clobber's user's provided version #54

Open
brokenalarms opened this issue Oct 18, 2016 · 4 comments
Open

Comments

@brokenalarms
Copy link

brokenalarms commented Oct 18, 2016

This was already addressed here originally in PR #44 . but then taken out to simplify that PR. To paraphrase:

We use our own custom onBeforeElUpdated function for memoizing sub-components, but then lose the ability to copy events. We'd need to be able to use both, ideally running the user function before the copier (since the user returning false immediately would make the event copier operations redundant).

I've seen the memoization example from the "choo stateful playground" example (though we aren't using choo directly), but it would be better at least for our purposes to have the isSameNode check internalized once in our main node comparison, rather than needing to include customElement.isSameNode = () => true; comparisons externally at the root of every one of our memoized sub-components (though only after the first update, which again adds more closure boilerplate if the same component is being used multiple times).

If there would be a way to get in @kristoferjoseph s work via this issue that would be great :)

@kristoferjoseph
Copy link
Contributor

kristoferjoseph commented Oct 20, 2016

@brokenalarms does passing an options object with events set to false also fix this issue?
It would look like:

{
  events: false,
  onBeforeElUpdated: function yourFunc (to, from) {
    console.log("Only do my stuff")
  }
}

You would need to do some sort of conditional check and prepare an options object to pass when calling update.

@kristoferjoseph
Copy link
Contributor

Reason I ask is because the initial thinking behind currying was two fold:

  1. A user could pass along their own function that could be run as well as the event copying.
  2. I could use this functionality to test out other updates that I might think I need.

@brokenalarms
Copy link
Author

@kristoferjoseph I replied via email but I don't think it worked, so sorry if this comes through twice.

  1. Yes, this runs the user onBeforeElUpdated but not copier. Setting opts.events = false (if I didn't want events) should still cause expected behavior with suggested code since yo-yo will just pass the opts object directly through to morphdom, which contains the user onBeforeElUpdated.

But that's kind of the point of yo-yo, otherwise I'd just use morphdom directly :) I want both hence the point of currying / this issue. I'll add a comment on the PR because I don't feel the main use case is still being satisfied there. Thanks!

@brokenalarms
Copy link
Author

I didn't really consider not loading events (since that's the point of
yo-yo, otherwise I'd just use morphdom directly). But if events was
false, then the user onBeforeElUpdated is already just present in the
opts object which is directly passed through to morphdom so will run as
normal.

Yes, the events copying should run as well as the user function - in the
code I supplied that happens if the user check returns true.

Are you talking about a use case where the user users their own function to
determine they don't want to update the element.. But still want the events
copied over again? That would seem to contradict the user not wanting the
DOM updated at all.

On Thu, Oct 20, 2016, 9:01 AM kj [email protected] wrote:

Reason I ask is because the initial thinking behind currying was two fold:

  1. A user could pass along their own function that could be run as well
    as
    the event copying.
  2. I could use this functionality to test out other updates that I might
    think I need.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#54 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AHC8CvW8Vo-n4DncSWsjLjjRxlLIWB1kks5q15BNgaJpZM4KaDzi
.

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

No branches or pull requests

2 participants