choojs / nanocomponent

🚃 - create performant HTML components
https://leaflet.choo.io/
MIT License
366 stars 30 forks source link

Add .rerender method internal re-renders #52

Closed bcomnes closed 7 years ago

bcomnes commented 7 years ago

This adds a this.rerender() method that re-renders using the last arguments passed to render.

bcomnes commented 7 years ago

Should we expose this.arguments instead of this._arguments? That way you can use it in the update function without feeling bad.

lrlna commented 7 years ago

@bcomnes mmmm i'd leave the this._arguments as 'private' tbh; keeps people from doing wonky things they shouldn't be doing. (e.g. I was doing a lot of stuff with the render cycle I shouldn't be doing, and don't want to be given more opportunity to do so haha)

bcomnes commented 7 years ago

Some issues with this:

<bret> Bret Comnes ok so the rerender method looks good, but there is one problem
08:43 if you update state internally, your update function may block the rerender
08:44 and there isn't an automatic way to reference last state, since internal state is an undefined concept
08:44 https://github.com/choojs/nanocomponent/pull/52/files#diff-7f86cbd5c7003482f69d1740ced2d693R27
08:47 ⇐ moszeed quit (uid230514@gateway/web/irccloud.com/x-xrlxmgtiasulpueo) Quit: Connection closed for inactivity
08:49 <bret> Bret Comnes lrlna: yoshuawuyts ^^
08:50 <lrlna> bret: oooof update blocking re-render is bad
08:51 <bret> Bret Comnes lrlna: sooo, two idea
08:51 ideas
08:51 we introduce the concept of state
08:51 and the caller of re-render passes newState to rerender as an argument
08:51 that way there is separation of new and existing state
08:52 or
08:52 rerender bypasses update
08:52 or the third, write getters for referencing state in the Dom and use that in your update function
08:53 which is weird
08:53 <yoshuawuyts> what do you mean by "update state internally"
08:53 <bret> Bret Comnes yoshuawuyts: I mean like this.somestate = 'foo' ; this.rerender()
08:53 state has changed at that point
08:53 <yoshuawuyts> ah
08:54 <bret> Bret Comnes or we could use a vector clock
08:54 <yoshuawuyts> sounds like we're making something complicated
08:54 perhaps a step back
08:54 which problem is this method supposed to solve?
08:54 <bret> Bret Comnes not having to call this.render(this.cachedArgs)
08:55 <yoshuawuyts> ok
08:55 <bret> Bret Comnes which still has the same problem
08:55 <yoshuawuyts> and we're now establishing that there's edge cases with doing that right?
08:56 <lrlna> hmmm i am not sure about introducing state, i am going to need to think about it :/ i currently pass down my choo state to elements if i need to, and store anything i need for the component on it's prototype. I feel like that works fairly well
08:56 <yoshuawuyts> hm
08:58 bret: I believe this arose from the fact that re-rendering from inside the "beforerender" method would break
09:00 <bret> Bret Comnes right
bcomnes commented 7 years ago

This has problems, don't merge yet.

bcomnes commented 7 years ago

I used a mutex to bypass the update method, forcing a full re-render. While this has some implications for components doing trickery in the update method, in general having this ability seems valuable.

bcomnes commented 7 years ago

I'm happy with this. Out of town for the next few days: If anyone want this sooner and there are no concerns, feel free to merge tag and release.

I should write this down in CONTRIBUTING but: