WebReflection / hyperHTML

A Fast & Light Virtual DOM Alternative
ISC License
3.07k stars 113 forks source link

Cached child components rendering issues #225

Closed yuretz closed 6 years ago

yuretz commented 6 years ago

We are trying to implement a reusable tab control component, and we've run into a couple of issues.

  1. Please take a look at this sample implementation. The problem with it is that it doesn't seem to render tab contents (TextView) the first time, but any subsequent update (e.g. after a button click) will re-render everything correctly. The work-around is not to cache the tab contents components and instead of TextView.for(state) always call new TextView(state), and everything starts working properly from the first render. However this feels a bit sub-optimal, since it will re-create all content components every time parent state is updated, even if the content's state doesn't change. And we can't understand why the original approach with caching wouldn't work too. What are we doing wrong?

  2. Please take a look at another sample implementation, where we try to cache everything (including content components) through Component.for(), so that all the child components are only created once. It seems to work properly, however the implementation is a little awkward, because we can only use "external" part of the state as a cache key, and everything else (event handlers, internal state, like selection index, etc.) has to be maintained separately, if we want to avoid unnecessary child components re-creation. What can we do to improve this code? Is there a better or more idiomatic way to dynamically attaching event handlers and passing event from a child to its parent component (preferably without re-creating the child)?

Thanks in advance for your answers!

WebReflection commented 6 years ago

Can you please check this version of 1 ? There is something weird going on with TabContent.for(item, index) needing the index, and I need to double check, but there are overall simplifications such:

Did my counter example help at all?

yuretz commented 6 years ago

Thanks a lot for the explanation and your comments, they definitely help!

Regarding TabContent.for(item, index), does it make sense to substitute it with TabContent.for(item, TabContent), to avoid cache clash with TabButton.for(item)?

WebReflection commented 6 years ago

@yuretz I think you can do that, but I see a bigger issue here. The item used through ClassA.for(item) should never be the same of the item used by ClassB.for(item) because even if the item is the same, the resulting instance would obviously not.

Accordingly, it looks like there's a missing link inside the Component.for logic, so while you can use TabContent.for(item, TabContent) to solve that case, I want instead fix this in a way that would never surprise you or even me.

Basically, I'd like to make the following:

TabButton.for(item, TabButton)
TabContent.for(item, TabContent)

implicitly the same of writing:

TabButton.for(item)
TabContent.for(item)

Classes matter here.

yuretz commented 6 years ago

This solution would be totally perfect, thank you!

I think I've now got a much better idea on how to solve these cases in a more proper and optimal way. However I still don't understand why wouldn't my first sample implementation (even being very redundant and not optimal at all) would not render properly the first time? What do I miss?

WebReflection commented 6 years ago

@yuretz the issue is the following one: try to use Tabs.for(this.state) instead of new Tabs(this.state) or use the do-not-render boolean in the ParentView constructor: this.setState(state, false);

Basically, since setState implicitly render the content, when you create new Tabs(state) all references within that state are used once to create the content.

However, when the bind renders the parent view, .render() is called again, but the state didn't change, nothing actually changed at all, so whatever was there it's still there.

But "where is there" ? Well, it's referenced to the first Tabs instance created within the constructor. The whole Component.for story was indeed to avoid creating new instances all over, because if you have nodes weakly referenced to some info and that info does not change, nothing would ever happen.

"But why don't I see that content"? Because nodes are inside the content of the first Tabs.

If you had new references for your state, the story would be different. You'll have new content each time, but this is not really what you want.

What you want is to be sure that anything rendered through components is never created at runtime if its content is related to some data that does not change.

As last proof of what I am saying, if you wrap config and models in a function, let's call it newState() and you return config from that function, using new Tabs(newState()) inside the preview would create new content each time, hence it would work.

Last, but not least, writing the following would also work as expected:

class ParentView extends Component {
  constructor(state) {
    console.log('ParentView.constructor()');
    super();
    this.tabs = new Tabs(state); // cache it once
    this.setState(state);
  }
  render() {
    console.log('ParentView.render()');
    return this.html`<p>Try clicking buttons below</p>
<div>${this.tabs}</div>`;
  }
}

But that's not really more elegant than this:

class ParentView extends Component {
  constructor(state) {
    console.log('ParentView.constructor()');
    super();
    this.setState(state);
  }
  render() {
    console.log('ParentView.render()');
    return this.html`<p>Try clicking buttons below</p>
<div>${Tabs.for(this.state)}</div>`;
  }
}

where this.state is always a unique object associated to its own component owner.

yuretz commented 6 years ago

Wow, thanks again for the explanation! Now things are definitely more clear. Somehow I missed the fact that bind() of course renders the whole view, and get these Tabs created twice. I've learned a few hyperHTML tricks today! šŸ˜„ šŸ‘