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

Revert on-load attribute special case behaviour #65

Merged
merged 1 commit into from
May 22, 2017
Merged

Revert on-load attribute special case behaviour #65

merged 1 commit into from
May 22, 2017

Conversation

greghuc
Copy link
Contributor

@greghuc greghuc commented May 3, 2017

This PR reverts the special case behaviour added in "morph onload tags on root": so the root element on-load attribute value is not maintained from oldTree to newTree.

Why this change?

  1. The current special case behaviour is breaking onUnload callbacks set up by the on-load library. Explanation below.
  2. Nanomorph can't be a generally applicable library if it has special cases like this. The README says that nanomorph can "guarantee a consistent, out-of-the box experience for all your diffing needs". This special case definitely isn't consistent :-). It threw me for a while: I couldn't explain why onUnload callbacks weren't firing after I switched from morphdom to nanomorph.
  3. If the intent of the special case behaviour is to enable onload/onunload lifecycle events for an html component that re-renders on data changes (with no on-load events on re-renders), then on-load does allow for this without the special case. Explanation below.

What's the issue with onUnload callbacks not firing?

This explanation requires understanding of the on-load library's source code..

For a given element, the on-load library updates the onloadid attribute value on every call to onload(el, on, off, caller), say from 'o1' -> 'o2'. This attribute value change is seen by on-load's MutationObserver, which then calls the off (onUnload) function associated with 'o1' (and the on (onLoad) function associated with 'o2').

The special case behaviour copies the onloadid attribute value from oldTree's root element to newTree's root element ('o1' -> 'o1'). If newTree already has an onloadid attribute (e.g. 'o2'), this is overwritten (with 'o1'). Since the root element's attribute value remains the same (always 'o1'), the MutationObserver doesn't fire, and the onUnload callback isn't called.

Thoughts on html components

I'm not sure why this special case was added in the first place. My guess is that it aims to help with the lifecycle of an html component that re-renders on data changes:

  1. creation: an html component is rendered as renderedElement, and added to the dom. onload is called as we want the onLoad callback to fire:
renderedElement = bel'<span>${data}</span>'
onload(renderedElement, onLoad, onUnload, caller)
component = renderedElement
document.body.appendChild(component)
  1. re-render: the component is re-rendered on data changes, with onload called as part of the process. But we don't want any on-load callbacks to fire as we're just re-rendering the same component. So we introduce the special case of maintaining the current onloadid attribute value between old and new versions of component, ignoring the new value from renderedElement:
renderedElement = bel'<span>${data}</span>'
onload(renderedElement, onLoad, onUnload, caller)
nanomorph(component, renderedElement)
  1. deletion: the component is removed from the dom. The onUnload callback fires:
component.parentNode.removeChild(component)

If the special case was to avoid on-load callbacks on component re-renders, then there is a better way: for all onload(renderedElement, on, off, caller) calls made for the component, use the same unique-id for caller. The on-load library skips callbacks when the onloadid attribute value changes for an element, but the associated caller value stays the same. See sameOrigin function at: https://github.com/shama/on-load/blob/master/index.js#L55

Hope this makes sense!

@yoshuawuyts
Copy link
Member

It definitely does! - See #63; we'll need to also fix this behavior upstream for this to work, but yeah agree (:

@yoshuawuyts yoshuawuyts mentioned this pull request May 8, 2017
15 tasks
@greghuc
Copy link
Contributor Author

greghuc commented May 8, 2017

@yoshuawuyts cool re removing special casing on #63. After seeing your remove onload pr for bel, I started using onload with bel directly, and it worked fine.

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay

@yoshuawuyts
Copy link
Member

v5.0.0

bcomnes added a commit that referenced this pull request Mar 3, 2018
Adds half of fix for nanocomponent #65
finnp pushed a commit to finnp/nanomorph that referenced this pull request Dec 14, 2018
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