max-mapper / yo-yo

A tiny library for building modular UI components using DOM diffing and ES6 tagged template literals
1.33k stars 65 forks source link

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

Open brokenalarms opened 7 years ago

brokenalarms commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 notifications@github.com 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 https://github.com/maxogden/yo-yo/issues/54#issuecomment-255149559, or mute the thread https://github.com/notifications/unsubscribe-auth/AHC8CvW8Vo-n4DncSWsjLjjRxlLIWB1kks5q15BNgaJpZM4KaDzi .