Adds currying of user supplied onBeforeElUpdated. #55

kristoferjoseph · 2016-10-18T18:51:24Z

No description provided.

kristoferjoseph · 2016-10-18T18:57:02Z

index.js

@@ -8,29 +8,32 @@ module.exports = bel
 module.exports.update = function (fromNode, toNode, opts) {
  if (!opts) opts = {}
  if (opts.events !== false) {


@shama @maxogden @yoshuawuyts Is this opts.events check needed?

You can set it to false to avoid copying any events

Ah, OK.
Was thinking that we could write this so that a person could return early from the onBeforeElUpdated and skip the events copying.

Turns out this request requires both.

kristoferjoseph · 2016-10-18T18:57:24Z

fixes #54

brokenalarms · 2016-10-19T21:34:31Z

index.js

@@ -8,29 +8,32 @@ module.exports = bel
 module.exports.update = function (fromNode, toNode, opts) {
  if (!opts) opts = {}
  if (opts.events !== false) {
-    if (!opts.onBeforeElUpdated) opts.onBeforeElUpdated = copier
+    if (!opts.onBeforeElUpdated) opts.onBeforeElUpdated = currier(opts.onBeforeElUpdated, opts)


This PR is still using the currier only if onBeforeElUpdated is -not- provided by the user.

Expected:

opts.onBeforeElUpdated = (opts.onBeforeElUpdated) ? currier(opts.onBeforeElUpdated, opts) : copier;

The function currier is also immediately calling the update function on first assignment, instead of returning a function that calls update then copier (provided the update function has not returned false).

EDIT: The first t.ok test would pass (incorrectly, if user onBeforeElUpdated was provided) due to update being called on the curry rather than the actual element comparison. However I really must be missing something as I can't understand how the other test would be passing, since line 11 should currently never run either currier or the hidden copier due to the user onBeforeElUpdated being supplied..

But I would have expected:

function currier(update, opts){ return function (f, t){ const userUpdatePassed = update(f, t); if (userUpdatePassed) return copier(f, t); return false; // or omit if morphdom doesn't care about undefined/false distinction } } function copier(f, t){}

Thanks!

You are right! Fixing.

brokenalarms

I don't understand this use case, whereby you provide your own onBeforeElUpdated, to determine whether a node should update - but then regardless of the outcome of this (e.g., "no! I don't want this element to update"), the rest of the content of copier runs regardless, clobbering the element I have specified to preserve.

If my user onBeforeElUpdated returns false, then I've already done what I needed to to that node and wouldn't then want events copied over again as this would contradict/go behind my check's "back" 🗡. If it should update though, then by all means copy across the events while you're at it, since that is the purpose of the yo-yo module.

The only way I'd see to supporting both is via a flag (i.e., a) I'm providing my own comparison function, which the copier function should also respect/chain through to; or b) even if I say I don't want the node updated, the copier function should run anyway). But I really don't see this second combination (as implemented in this PR) alone as solving the issue at hand, or ever really being desired behavior (but if someone else has this as a valid use case then just LMK).

(As discussed in the issue, the third case is when opts.events is set to false, in which case there is no further logic needed as the opts object is just passed through untouched to morphdom, where the user's onBeforeElUpdated will be the sole comparator if present.)

Cheers!

kristoferjoseph · 2016-10-20T18:12:42Z

Here is the thinking behind adding a currier:

A user only wants to run their own onBeforElUpdated and not have `yo-yo` do any work.

in this case pass opts: {events: false, onBeforElUpdated: myUpdateFunc} to yo.update and skip copying events.

This code would require a user to pass the above options to yo.update(from, to, {events: false, onBeforElUpdated: myUpdateFunc})

OR

A user wants `yo-yo` to copy events as well as run their own `onBeforeElUpdated` pass

in this case pass opts: {onBeforElUpdated: myUpdateFunc} and run an update pass as well as copy events )

This PR covers both those intended use cases.

Does this make sense?

Your intended use?

If I am hearing you correctly, you have a third use case in where you would like to determine if you need yo-yo to copy events with your own onBeforeElUpdated?

Thank you ahead of time.

brokenalarms · 2016-10-20T18:47:43Z

Hey, thanks for your work on this.

yep, that works - (although that's just the same as just using morphdom; you wouldn't need yo-yo).
The purpose of onBeforeElUpdated is to specify whether morphdom should update the from element with the to element at all:

Called before a HTMLElement in the from tree is updated. If this function returns false then the element will not be updated.

yo-yos copier essentially does a little hack whereby it's performing directly surgery on the to element at this point, since it is being made available, and then returns undefined. Because morphdom checks explicitly for false as the return of this function:

            if (onBeforeElUpdated(fromEl, toEl) === false) {
                return;
            }

...it ignores the result of this function and continues updating down the tree, but with events now in place.

The issue is therefore that copier removes the actual point of this function from the user - determining whether a node should update. If I don't want a node to update, then that doesn't / shouldn't really include a special exemption for the copying across of events! :) As implemented in your PR, yo-yo is still taking away the actual point of onBeforeElUpdated from me, allowing me to do my own surgery to the from node if desired, but not actually signifying correctly to morphdom that I do not want the element updated.

Is that any clearer? I appreciate that given how yo-yo's copier manipulates the element, this may have hidden the actual original point and use of morphdoms onBeforeElUpdated utility.

brokenalarms · 2016-10-20T18:59:03Z

Personally, for my case, I don't especially care if events get re-copied, even if I had returned false from onBeforeElUpdated (because whilst my components should not be updated, e.g because it's a memoized sub-component that has asynchronously fetched and appended an svg after instantiation, re-copying events at the root level of my sub-component won't actually break that sub-component).

However, at best re-copying is a waste of cycles (again, I don't care about this though), but at worst I do actually have some event I don't want changed which I would lose control over deciding. (NB this is not the same as setting opts.events = false. I do want the events copied over the first time, for sure, that's the point of yo-yo! I just don't need them re-copied once the sub-component is instantiated.)

This point is relatively minor - but the main thing is that in your PR, I still cannot stop morphdomfrom continuing to update children beyond that point, which is what onBeforeElUpdated is for. Hope that clears it up! :)

kristoferjoseph · 2016-10-20T19:26:41Z

Makes perfect sense.

Just wanted to make sure I understood the intention and that it was spelled out appropriately in the PR discussion.

Thanks!

kristoferjoseph · 2016-10-21T15:58:45Z

test.js

+    events: false,
+    onBeforeElUpdated: function(from, to) {
+      t.ok('User supplied onBeforeElUpdated called')
+      return false


Returning false from user supplied onBeforeElUpdated will skip yo-yo's copying of events.

brokenalarms · 2016-10-21T18:44:21Z

index.js

+    // that can be set via attributes
+    return function copier (f, t) {
+      var copyEvents = userUpdate? userUpdate(f, t): true
+      if (!copyEvents) { return false }


morphdom only stops the update if the onBeforeElUpdated function explicitly returns false. This falsy check will incorrectly stop the copier from running if the user-provided onBeforeElUpdated function returns undefined.

(The copier function itself returns undefined, yet the node swapping continues downward because false wasn't returned).

The code needs the explicit check if (copyEvents === false) return false, same as is present in morphdom.

Cheers

brokenalarms · 2016-10-22T01:22:56Z

Ok, that works for me. I still need to manually copy across those attributes I'm interested in that trigger updates in the fromEl webComponent from the "potential" toEl element, and then discard the toEl, but this way my sub-component is memoized and doing its own thing.

kristoferjoseph · 2016-10-25T19:00:26Z

@shama @yoshuawuyts What ya'll think?

yoshuawuyts · 2016-11-02T21:47:38Z

@kristoferjoseph hey, this has been in my inbox for a while now and I've given it a few shots but not sure I follow what's going on. Could you give me the tl;dr of what's going on? thanks! 😁

kristoferjoseph · 2016-11-02T21:52:55Z

@yoshuawuyts tl;dr

This PR supplies a way for a user to provide their own onBeforeElUpdate method that will be called before yo-yo copies events.

This allows a user to run their own update routine then decide if they want to copy events.

In the case they have determined during their update routine that no further updates are needed they can return false.

yoshuawuyts · 2016-11-02T22:03:39Z

Hmm, wouldn't this be a legit reason to drop down into morphdom, pull in
the standalone copy hook module
and then also register any other modules. I quite like the idea of having
yo-yo be this non-configurable thing
that mostly just works. - not sure if that's technically possible tho; also
I don't believe we're using that third party
hook package in yo-yo (yet) so there might be a chance of getting out of
sync.

On Wed, Nov 2, 2016 at 10:52 PM kj [email protected] wrote:

tl;dr

This PR supplies a way for a user to provide their own onBeforeElUpdate
method that will be called before yo-yo copies events.

This allows a user to run their own update routine then decide if they
want to copy events.

In the case they have determined during their update routine that no
further updates are needed they can return false.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#55 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACWlenuq16Jf6j57KYzKetV2b515kFzBks5q6QY4gaJpZM4KaKBL
.

kristoferjoseph · 2016-11-02T22:25:26Z

@yoshuawuyts your response is similar to my initial thoughts. You may need to read this issue thread to get the bigger picture.

brokenalarms · 2016-11-02T23:27:42Z

In the meantime I've done just that and needed to bring this code into my
own project.

The docs say that yo-yo passes your options through to morphdom. This is
not the case here, as yo-yo changes the expected behavior of morphdom
opts as a side effect of adding the events copier.

It's not about configuring yo-yo or changing its behavior at all - by using
it on its own (separate to choo) its function is just to augment morphdom
with events copying ability. Without this PR you gain the ability to have
events copied but then get one of your passed through options to morphdom
arbitrarily blocked.

On Wed, Nov 2, 2016, 3:25 PM kj [email protected] wrote:

@yoshuawuyts https://github.com/yoshuawuyts your response is similar to
my initial thoughts. You may need to read this issue thread to get the
bigger picture.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#55 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AHC8Co37j5of91OFUpeo-uUXD8x-UkjEks5q6Q3YgaJpZM4KaKBL
.

Adds currying of user supplied onBeforeElUpdated.

33e787f

kristoferjoseph commented Oct 18, 2016

View reviewed changes

Removes mistaken options events usage.

5a506d1

brokenalarms suggested changes Oct 19, 2016

View reviewed changes

Update reverts to original curry approach.

5f5cdca

brokenalarms suggested changes Oct 20, 2016

View reviewed changes

Update adds @brokenalarmss feedback

8658c76

kristoferjoseph commented Oct 21, 2016

View reviewed changes

brokenalarms suggested changes Oct 21, 2016

View reviewed changes

Patch explicitly returns false to match morphdom API.

f18cdf4

kristoferjoseph closed this May 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds currying of user supplied onBeforeElUpdated. #55

Adds currying of user supplied onBeforeElUpdated. #55

kristoferjoseph commented Oct 18, 2016

kristoferjoseph Oct 18, 2016

shama Oct 18, 2016

kristoferjoseph Oct 18, 2016

kristoferjoseph Oct 21, 2016

kristoferjoseph commented Oct 18, 2016

brokenalarms Oct 19, 2016 •

edited

Loading

kristoferjoseph Oct 19, 2016

brokenalarms left a comment •

edited

Loading

kristoferjoseph commented Oct 20, 2016 •

edited

Loading

brokenalarms commented Oct 20, 2016 •

edited

Loading

brokenalarms commented Oct 20, 2016 •

edited

Loading

kristoferjoseph commented Oct 20, 2016

kristoferjoseph Oct 21, 2016

brokenalarms Oct 21, 2016

brokenalarms commented Oct 22, 2016

kristoferjoseph commented Oct 25, 2016

yoshuawuyts commented Nov 2, 2016

kristoferjoseph commented Nov 2, 2016 •

edited

Loading

yoshuawuyts commented Nov 2, 2016

kristoferjoseph commented Nov 2, 2016

brokenalarms commented Nov 2, 2016

Adds currying of user supplied onBeforeElUpdated. #55

Adds currying of user supplied onBeforeElUpdated. #55

Conversation

kristoferjoseph commented Oct 18, 2016

kristoferjoseph Oct 18, 2016

Choose a reason for hiding this comment

shama Oct 18, 2016

Choose a reason for hiding this comment

kristoferjoseph Oct 18, 2016

Choose a reason for hiding this comment

kristoferjoseph Oct 21, 2016

Choose a reason for hiding this comment

kristoferjoseph commented Oct 18, 2016

brokenalarms Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

kristoferjoseph Oct 19, 2016

Choose a reason for hiding this comment

brokenalarms left a comment • edited Loading

Choose a reason for hiding this comment

kristoferjoseph commented Oct 20, 2016 • edited Loading

A user only wants to run their own onBeforElUpdated and not have yo-yo do any work.

A user wants yo-yo to copy events as well as run their own onBeforeElUpdated pass

Your intended use?

brokenalarms commented Oct 20, 2016 • edited Loading

brokenalarms commented Oct 20, 2016 • edited Loading

kristoferjoseph commented Oct 20, 2016

kristoferjoseph Oct 21, 2016

Choose a reason for hiding this comment

brokenalarms Oct 21, 2016

Choose a reason for hiding this comment

brokenalarms commented Oct 22, 2016

kristoferjoseph commented Oct 25, 2016

yoshuawuyts commented Nov 2, 2016

kristoferjoseph commented Nov 2, 2016 • edited Loading

yoshuawuyts commented Nov 2, 2016

kristoferjoseph commented Nov 2, 2016

brokenalarms commented Nov 2, 2016

brokenalarms Oct 19, 2016 •

edited

Loading

brokenalarms left a comment •

edited

Loading

kristoferjoseph commented Oct 20, 2016 •

edited

Loading

A user only wants to run their own onBeforElUpdated and not have `yo-yo` do any work.

A user wants `yo-yo` to copy events as well as run their own `onBeforeElUpdated` pass

brokenalarms commented Oct 20, 2016 •

edited

Loading

brokenalarms commented Oct 20, 2016 •

edited

Loading

kristoferjoseph commented Nov 2, 2016 •

edited

Loading