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

remove onload #76

Merged
merged 1 commit into from
May 19, 2017
Merged

remove onload #76

merged 1 commit into from
May 19, 2017

Conversation

yoshuawuyts
Copy link
Member

Replaces #69 - making this part of the semver major in #74

@yoshuawuyts yoshuawuyts merged commit 3370966 into choojs:master May 19, 2017
@yoshuawuyts yoshuawuyts deleted the rm-onload branch May 19, 2017 15:01
@yoshuawuyts
Copy link
Member Author

v5.0.0

@timwis
Copy link
Member

timwis commented May 21, 2017

Hey is there a thread or background somewhere on this? Just curious.

@yoshuawuyts
Copy link
Member Author

@timwis think this has mostly gone over IRC / in person. Docs might be lacking, so here's my take on why this was a good idea:

It's become pretty apparent in recent months that using onload in combination with DOM diffing requires a fair few hoops to go through. Namely, in order for onload to work the same DOM node must stay on the DOM between renders. And if any DOM node is rendered in two DOM hierarchies at the same time, it will unmount from one of the locations - which in this case would trigger onload to fire when it shouldn't.

To make going through these hoops easier, we created packages like nanocomponent, microcomponent and cache-component.

But despite this people have still been using the onload attribute, often leading to rather surprising results. I thought it made sense to remove this footgun, so I consulted in person with @shama in January during the DonutJS meetup if removing this made sense - and I got the thumbs up. This process was perhaps not as transparent as it should have been, and I apologize for that.

Hopefully this all makes sense. I truly hope that this will cause people to run into less unexpected behavior, and allow us as a community to focus on building robust, reusable components.

Let me know if I missed anything, happy to answer! :D

@bengourley
Copy link

How often is the following really the case? 🤔

the same DOM node must stay on the DOM between renders. And if any DOM node is rendered in two DOM hierarchies at the same time, it will unmount from one of the locations - which in this case would trigger onload to fire when it shouldn't.

I use onload/onunload a fair amount in my app for a couple of use cases:

  1. Showing modals, assigning keyevents for enter esc:
    <div class="modal" onload=${el => window.addEventListener('keyup', fn)} onunload=${el => window.removeEventListener('keyup', fn)}>
  2. Triggering an action that will load some content if required:
    <main onload=${el => send('loadFromUrl')}>

It's a convenience thing to have onload and onunload baked in as these _pseudo_attriubute niceties… I guess my two questions are,

  1. are my use cases valid, and/or is there a better way to perform these actions?
  2. should my solution be to just import onload into my program and use it directly?

Cheers :D

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented May 22, 2017 via email

@bengourley
Copy link

1: if a re-render happens at any point in
the tree the modal is mounted in, it will run another set of lifecycle
events and yield unexpected results.

Ah this makes more sense to me, so if any parent of a node with onload is replaced, even though the actual node wouldn't have changed, it appears to be a new node so does run?

  1. The solution should be to use one of the libs I linked above, possibly
    even to generate a new, reusable lib. Using on-load directly is indeed a
    direct replacement, but wouldn't fix the bugs.

I'm just trying to understand how the bug is fixed, so no hurry on your reply! Looking at nanocomponent for reference, is it because of the unique id being assigned to the node, used in conjunction with onload that makes it work?

Thx 👌

@greghuc
Copy link
Contributor

greghuc commented May 22, 2017

@bengourley I got onload working with bel directly (without nanocomponent, etc). The key to using it correctly is to call onload(renderedElement, onLoad, onUnload, caller) with a unique caller value. So you generate caller once for an html 'component' that you create (e.g. 'rand12343'). Your component may be re-rendered multiple times due to underlying data changes, and the underlying dom may change if you use dom-diffing with nanomorph/morphdom. But onload will behave correctly re calling onload/onunload handlers, provided you always call onload on re-renders with the same caller value, for the lifetime duration of your html component.

It's kind of hard to explain onloads behaviour. I made an attempt here: choojs/nanomorph#65 (comment). Take a look at: "Thoughts on html components".

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.

4 participants