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

lifecycle hooks #1

Closed
yoshuawuyts opened this issue May 13, 2016 · 40 comments
Closed

lifecycle hooks #1

yoshuawuyts opened this issue May 13, 2016 · 40 comments
Milestone

Comments

@yoshuawuyts
Copy link
Member

Every once in a while we need hooks to manage things like animations:
max-mapper/yo-yo#18

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented May 23, 2016

For managing simple animations, setting classes is probably the way to go. Using classnames might prove to be helpful.

For managing complex animations people should probably use a finite state machine such as f1.

For reaction to DOM nodes being removed (e.g. trigger an event to load / unload data) use MutationObservers (caniuse).

See Also

@yoshuawuyts
Copy link
Member Author

@timwis
Copy link
Member

timwis commented May 27, 2016

What about firing an ajax request when a view is shown? Can't just put it in the view logic because then it fires again every time the state changes, and you end up with an endless loop if that ajax changes the state. In react, you'd use componentDidMount. What do you recommend with choo?

EDIT: Another thing that makes this hard is the on state change handler, which calls yo.update(oldTree, newTree) even when there is no "old tree" because it's the first time the view's rendering.

@yoshuawuyts
Copy link
Member Author

What about firing an ajax request when a view is shown?

Untested, but I reckon on-load does the job correctly. Given that unless the component has mutated it should run just fine. I'd hook a send() call to the event. If for some reason this turns out to be a bit unreliable, making the action itself idempotent is recommended so that even if the action fires nothing bad happens.

even when there is no "old tree"

What do you mean? There should always be a tree - or is this a conflict between the first call to fetch data and the tree being rendered? If this is a bug I'd definitely want to fix this - could you clarify? Thanks!

@timwis
Copy link
Member

timwis commented May 28, 2016

making the action itself idempotent is recommended so that even if the action fires nothing bad happens.

In my case, the action is "fetch a list of tables," which is idempotent. The problem is that once they're fetched, the state changes, so the view re-renders, which triggers the fetch, the state change, the re-render, and an endless loop :P

I reckon on-load does the job correctly

Interesting -- do you mean something like this?

module.exports = (params, state, send) => {
  const tree = choo.view`<div>Foo</div>`
  onload(tree, () => send('tables:fetch'))
  return tree
}

If so, I may be missing something, because it doesn't appear that changing views triggers the onload library.

What do you mean? There should always be a tree

I tried to get around the "endless loop" using this approach:

module.exports = (params, state, send) => {
  if (!state.tables.fetched) send('tables:get')
  return choo.view`.....`
}

This works fine when you load another view and then navigate to this view. But if this view is the first view rendered, you get an error Cannot read property 'nodeType' of null. The stack trace leads you back to the line I linked above -- the problem is that it's trying to update the tree before there is a tree to update.

  1. Call view
  2. Fire tables:get event
  3. Async call fires tables:receive event
  4. receive handler updates state
  5. State update triggers yo.update()
  6. Initial render of view (this happens too late)

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented May 28, 2016

Interesting, I expected the on-load lib to just work - but from your comment it appears it isn't. I'll be super busy for the next 10 days, so probably can't get around for a repro, but if it on-load isn't working as expected I recommend opening an issue there. Hope that makes sense. Cheers!

@timwis
Copy link
Member

timwis commented May 28, 2016

I don't think it's an issue with on-load actually -- I put together a test case to illustrate what I believe to be the issue.

on-load isn't triggered when you use yo.update() because it's not actually adding the new <div> to the dom; it's doing its diffing thing. I believe that's what's occurring here in choo. That's why the initial page triggers on-load (because it's being added to the dom), but pages after that don't trigger it.

Let me know if you still think it's an on-load issue and I'll raise it there.

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented May 29, 2016

dammit - yeah, I see what you mean with your repro. Perhaps morphdom-hooks might get us where we want to be?

const hooks = require('./')
const morphdom = hooks(require('morphdom'))

function button () {
  const b = document.createElement('button')
  b.onadd = console.log.bind(console, 'button added to page!')
  b.onupdate = console.log.bind(console, 'button updated in page!')
  b.ondiscard = console.log.bind(console, 'button discarded from page!')
  return b
}

Ideally I reckon this would be part of yo-yo, but I reckon we could probably layer it inside of choo if needed. e.g. bel is for components, yo-yo is to manage the DOM update flow, choo is an architecture layer.

@timwis
Copy link
Member

timwis commented May 30, 2016

Maybe I'm forgetting something, but...don't we already know when the view is added to the page because it's being done by the router?

app.router((route) => [
  route('/', require('./views/connect')),
  route('/tables', (params, state, send) => {
    // fire "componentDidMount" logic here.
    // it shouldn't be re-run if state changes, because route hasn't
    return require('./views/tables')(params, state, send)
  })
])

It would be nice to do that inside the view somehow, but just pointing out that we kind of already have a hook mechanism.

EDIT: On further review, this line suggests that it would be called again on re-render

@timwis
Copy link
Member

timwis commented May 31, 2016

Just thought of another use case for lifecycle events: a library like Leaflet requires you to initialize it by passing it a DOM node or ID. (1) not sure how you'd do it with the current way views work (if you initialize before returning the markup, the container won't be in the DOM yet and leaflet won't work), and (2) any time the state changes, choo will re-render the entire view, which I imagine could be a performance issue. I believe react provides a few events for this (componentWillReceiveProps or componentWillUpdate or something).

@yoshuawuyts
Copy link
Member Author

Feel like we're also not leveraging intermediate lifecycle hooks (as shown in patrick-steele-idem/morphdom#65 (comment))


@timwis I'd memoize the DOM node, check if state has changed and then only re-render if it hasn't; basically always passing the same node. That should pretty much always work I reckon ✨

@timwis
Copy link
Member

timwis commented Jun 5, 2016

Update: thanks to #27, this approach is possible to enable something like making an ajax call the first time a route/view is triggered

@yoshuawuyts
Copy link
Member Author

Looks like shama/on-load#2 might be our savior

@timwis
Copy link
Member

timwis commented Jun 23, 2016

on-load seems to be working great for "loaded" and "unloaded." What are your thoughts on a "mutated" event? For example, if you're initializing a library and it needs to be re-initialized any time the data changes? I suppose we could compare state to prev. Trying to think of any other use cases where we might want to know when the view's DOM was changed.

@yoshuawuyts
Copy link
Member Author

yeah comparing state to prev is def the way to go for this one. on-load is pretty much just a wrapper around MutationObserver (which seems supported by 98% of browsers); anything other than these hooks the component should be responsible for handling itself I reckon ✨

@timwis
Copy link
Member

timwis commented Jun 26, 2016

Hey @yoshuawuyts hopefully I'm just doing it wrong but I was just trying to implement on-load and ran into unexpected behaviour with it. Perhaps I'm misunderstanding? Here's a simple test case. Shouldn't that work?

It seems like the second page doesn't trigger onload because it uses the same type of parent container (<div>). Even if you give that <div> different id or class, it doesn't trigger the second view. Only if you use a different element type (like <section>).

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Jun 26, 2016

@timwis no, you're right - it shouldn't - could be because of https://github.com/shama/on-load/blob/master/index.js#L15 not being cleaned up, thus maintaining the reference to the same DOM node; could you open an issue on on-load about this?

@knownasilya
Copy link

knownasilya commented Jun 28, 2016

I also have the Leaflet scenario, will on-load work for that? Would love an example here (and in the docs)..

@timwis
Copy link
Member

timwis commented Jun 29, 2016

Hey @knownasilya yes, it should -- I went to put together a demo but came across an issue that I've just reported ^ if you look at the demo there you can get an idea of how on-load works. Of course, pretty soon you'll be able to just use onload attribute in the markup rather than pulling in the on-load library.

@knownasilya
Copy link

Of course, pretty soon you'll be able to just use onload attribute in the markup rather than pulling in the on-load library.

Really looking forward to that!

@timwis
Copy link
Member

timwis commented Jun 30, 2016

Update for any subscribers to this issue, shama seems to have figured it out. I've posted on that thread with a couple demos. At this point, then, we're just waiting on:

I assume from there there won't actually need to be any changes to choo (other than documentation).

@knownasilya
Copy link

knownasilya commented Jun 30, 2016

Looks like bel has been updated and published, so yo-yo has access since it was a minor change.

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Jun 30, 2016

I guess that means that... we now have lifecycle hooks haha ✨ - should probably formally bump the dep in yo-yo tho, bump yo-yo here and update docs across the board. Probably officially make it part of choo 3.0 - but everyone can use it right now then 😁

@timwis
Copy link
Member

timwis commented Jun 30, 2016

Woohoo! Dep bump posted at max-mapper/yo-yo#34

Just to be clear to anyone stumbling upon this thread, you should now be able to use the onload and onunload attributes in your markup as documented in bel, ie:

const view = (params, state, send) => {
  return choo.view`<div onload=${() => console.log('loaded!')}></div>`
}

@jondashkyle
Copy link

Probably worth mentioning in the documentation that the onload and onunload attributes must be defined on the root element of the view, otherwise you’ll run into trouble! Took me a moment to realize it, but glad to have this freshly integrated. Very about choo, btw ❤️🚂

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Jul 4, 2016

@jondashkyle waiiit, what do you mean?

edit: derp, I see what you mean. Yeah reckon that's good to mention haha

@timwis
Copy link
Member

timwis commented Jul 4, 2016

Yikes, really? I think it's supposed to work with children elements :-/ at least that was what @yoshuawuyts had in mind, otherwise we could have just triggered it via the router...

You know, this is just a guess, but I think onload works via the function's .caller... so perhaps children elements would work if you put them in their own function.

@jondashkyle
Copy link

jondashkyle commented Jul 4, 2016

I put together a little demo repo as there are a few different states in which it’s breaking. There’s a definite possibility that I’m not using it correctly! Hope it’s helpful in some form:

https://github.com/jondashkyle/choo-onload/

Worth noting that I get the same errors when doing something which looks like this:

const html = require('choo/html')
const onload = require('on-load')

const view = (state, prev, send) {
  const div = html`
    <div>
      Should trigger an on-load event.
    </div>
  `

  onload(div, (el) => {
    console.log('Loaded')
  }, (el) => {
    console.log('Unloaded')
  })

  return div
}

Haven’t engaged an actively developing open source project like this before, so apologies if this would be better suited in the on-load or bel repositories. Would be down to do a little pull request to try looking into it. Worth repeating I’m mad jazzed w/ choo!

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Jul 4, 2016

Uh oh, that's not good. I thought by children you meant like:

html`<div><div onload=${}</div></div>`

which would've been acceptable I reckon. Hmmm, this def looks like a bug.

cc/ @shama any idea what could be up? I'm not super familiar with the on-load code yet

shama added a commit to shama/on-load that referenced this issue Jul 4, 2016
@shama
Copy link
Member

shama commented Jul 4, 2016

Nice error report @jondashkyle!

I've got a fix for the errors. The child onloads not firing is interesting. Basically morphdom isn't adding a <ul> tag but using the existing one and adding a data-onloadid attribute. We don't check childs on attribute changes. I'm going to play around with it a bit but likely the fix is going to be navigate child nodes on attribute changes as well.

@yoshuawuyts
Copy link
Member Author

Can I assume this is mostly resolved then? Are there still any issues - any way I can help?

shama added a commit to shama/on-load that referenced this issue Jul 6, 2016
@shama
Copy link
Member

shama commented Jul 6, 2016

@yoshuawuyts Yep all resolved! Fixed and released as on-load@3.1.1. Turned out to be just a bug. It monitors anytime a data-onloadid attribute gets added/removed/changed anywhere so I don't need to loop through child nodes. I just had a mistaken check only allowing nodes with previous data-onloadid's in, so if you had an existing node but add the data-onloadid, it would ignore it.

Test in bel specifically for this too: choojs/nanohtml@82d9652

@jondashkyle
Copy link

jondashkyle commented Jul 6, 2016

Understanding how morphdom works, this does appear to be functioning properly, but it deviates from my expectation. If you give this a try, I would anticipate the console to log One upon first load, and when clicking the link /two seeing Two in the console.

const choo = require('choo')
const html = require('choo/html')
const app = choo()

const one = (params, state, send) => {
  return html`
    <div onload=${() => console.log('One')}>
      <h2>one</h2>
      <a href="/two">/two</a>
    </div>
  `
}

const two = (params, state, send) => {
  return html`
    <div onload=${() => console.log('Two')}>
      <h2>two</h2>
      <a href="/one">/one</a>
    </div>
  `
}

app.router((route) => [
  route('/', one),
  route('/two', two)
])

const tree = app.start()
document.body.appendChild(tree)

My naive understanding of what’s happening: When Morphdom handles the mutation between the two views, it appears there hasn't been a change on the root div element as Bel has removed the function contained within the value of the onload attribute.

Additionally it’s worth noting that something like the following does work as expected, but you loose the optimizations of DOM morphing as both the root element and all children are replaced:

const one = (params, state, send) => {
  return html`
    <el-one onload=${() => console.log('One')}>
      <h2>one</h2>
      <a href="/two">/two</a>
    </el-one>
  `
}

const two = (params, state, send) => {
  return html`
    <el-two onload=${() => console.log('Two')}>
      <h2>two</h2>
      <a href="/one">/one</a>
    </el-two>
  `
}

If it sounds like I’m understanding this correctly I’m down to dive in and explore options or alternatives to solving this :)

MAJOR ups on the 3.0 release!

@shama
Copy link
Member

shama commented Jul 6, 2016

@jondashkyle Run npm cache clean && rm -rf node_modules && npm i and give it another go. Just published a fix to bel where on-load couldn't correctly identify the element. This should fix your above example.


I'd like to solve this problem better though. Currently I don't know a good way to identify an element creator with native elements. So when morphing existing DOM nodes, I rely on the Function.caller. Which basically asks, when the data-onloadid attribute changes, did the same function change it? If it's not the same function, fire the onload events.

In your example above, it thought the same Function.caller created both elements, so didn't fire the events.

This isn't full proof as I bet adding multiple onload attributes to childs of a function that creates an element then morphs those to where the nodes are not removed/added but only have their attribute changed, probably won't work. So eventually I'll need to explore a better way to handle this.

@shama
Copy link
Member

shama commented Jul 6, 2016

Also dropping a note here, Function.caller is not used with yo-yoify, since it's able to identify the element creator when transforming. So for production code, it will be just a string: shama/yo-yoify@8fd20e0

@yoshuawuyts
Copy link
Member Author

Can I conclude that this is now resolved, and that this issue can be closed? (Bit sick atm, haven't kept up with the whole discussion)

@shama
Copy link
Member

shama commented Jul 6, 2016

@yoshuawuyts There are still some edge cases I need to get sorted but if the current behavior is satisfactory for the group, then I think it's safe to close.

If there are more bugs, feel free to open on on-load and I'll get them sorted. Thanks @timwis, @knownasilya and @jondashkyle for helping me get this fine tuned!

Hope you feel better Yosh!

@sotojuan
Copy link
Contributor

sotojuan commented Aug 27, 2016

Status on this?

@yoshuawuyts
Copy link
Member Author

@sotojuan thunks are not great; the reason bugs exist is because any given DOM node can only live in one tree at the same time, so conflicts happen. This is heaps annoying and causes events to be called all over the place. I think the best solution would be to have a way to signal to morphdom that it should halt its diffing algorithm at some point; and then create a way to nest multiple diffing trees. There's IRC logs on discussions about this from a few days ago; here-ish. Hope this makes sense

@yoshuawuyts
Copy link
Member Author

Welp, now that patrick-steele-idem/morphdom#85 has been merged it's safe to say isSameNode() is the way forward; it should be working reliably now.

The handbook should get a section on element lifecycles, memoization and widgets soonish.

cache-element is being built which should allow for a clean API to manage updates and events in the choo setup.

I think all of this concludes this issue: lifecycle events exist and work. All that's needed now is to update the ecosystem, but it's no longer an issue. Thanks all for contributing in this thread 🎉 - we've made it!

goto-bus-stop pushed a commit to goto-bus-stop/choo that referenced this issue Aug 25, 2018
docs: fix module name in example
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

6 participants