Problem with data from a component being sent to another

[mairatma]

This example doesn't render the expected output when the button is first clicked. It will render 1 bar for the second element instead of just 1.

That's because some config data of the third component (which was the second one before the button was clicked) is being wrongly passed to it. Need to investigate and fix this.

[mairatma]

This works now :)

Just make sure you update metal-incremental-dom to rc.42 and metal.js to rc.4.

[bryceosterhaus]

I still seem to be getting the issue. Not with that example though. Let me see if I can nail down another example and share it with you

[bryceosterhaus]

In our project we are seeing issues with the code below

<a class="view-port">
        // playable is a boolean
    {playable &&
        <div class="play-overlay">
                         // Icon has default state of size="large"
            <Icon multiplier={3} name="play" />
        </div>
    }
</a>

<div class="controls">
    <div class="viewer-control right" data-onclick={this.handleNext}>
        <Icon display="secondary" multiplier={2} name="arrow-right-rod" size="small" />
    </div>

    <div class="viewer-control left" data-onclick={this.handlePrevious}>
        <Icon display="secondary" multiplier={2} name="arrow-left-rod" size="small" />
    </div>
</div>

<Icon /> nested within play-overlay results in a config of

{
     display: 'secondary',
     multiplier: 3,
     name: 'play',
     size: 'small',
}

It seems to be somehow picking up the config of the other two <Icon /> components, and it shouldn't.

This make sense?

[mairatma]

We talked offline about this, so I'll try to document the results of the conversation here.

It's not very easy to figure out if a component instance can be reused or not after an update. By default,
when a JSX call to a certain component constructor is made, Metal.js tries to match that to an existing component instance according to the order in which the calls are made.

That means that in the example above Metal.js would have created two Icon instances in the first render. Later, when playable becomes true, Metal.js will get three calls to Icon, and so the first two will match the existing two instances. This wouldn't cause a problem if Icon was using data from this.config directly, since that's cleared on every update, but this example uses this.display and this.size, as internal state properties, so those are not cleared between updates.

This can be fixed by giving more hints to Metal.js about how to reuse the components, through ref. ref is basically used for creating references to sub component instances inside the parent component, but it also allows Metal.js to know exactly to which instance a JSX call should be applied to. So adding ref to the <Icon /> JSX calls in this example would solve the problem.

That said, this use case wouldn't have required extra information if implemented using React. That's because they use a different approach for reusing components. They also use the order in which they appear, but not the order inside the whole parent component, they take into account the order inside the parent element instead. Since the conditional <Icon /> call is in a different parent, this would work correctly in React. If all 3 were inside the same parent, and Icon used internal state, React would also get things wrong though. In that case they request that you use key to help them reuse the correct instances.

When first solving this issue I studied React's approach and thought about changing Metal.js to follow it instead, but it would be a lot of work and I'm not entirely sure that it would be worth it. As I said, by using ref you can guarantee that we'll reuse components correctly, and our approach covers some cases that would require using key in React as well.

In the end I think we should take the opportunity that we now have more people using Metal.js to observe how often we need to use ref with the current approach, so that we can have more data to compare it with React's and decide if it's worth changing.

Let me know if anyone has any questions or suggestions about this!

[mthadley]

It seems like to be safe, as an application developer, you would probably always want to add a ref then. You may not always need it, but it would be easier than trying to debug an issue where Metal is not reusing your component how you would expect.

Unless it's obvious when you need it, which I guess would be when you have conditionally rendered sub-components? But even then, it seems somewhat error prone to me.

[mairatma]

It's safer to use ref when you have more than one component of the same type, that renders conditionally inside the same parent component.

As I said, React has this same problem, requiring you to use key, but in other use cases, when you conditionally render more than one component of the same type in the same parent element.

It's just impossible to guarantee 100% via jsx to which existing instance you're referring to. We can have logic to try to cover as many cases as possible, but without hints like ref and key it's impossible to catch every one's use case all of the time.

Basically, React's logic reuses components less, since they limit reuse within a parent element, but that also causes them to lose internal state of components more often, due to not reusing them, which would happen less with Metal.js. I suspect that React's logic may be better in most common cases, but since I'm not sure I'd rather not change this right now, but wait and observe a little longer instead.

[mthadley]

That makes sense. I still think it will be a "gotcha" for newcomers to the framework, but since we know why now, its not an urgent thing for us.

Update: I was talking with @bryceosterhaus, and I now realized I skimmed over your distinction between parent element and parent component. Either way, I think this will probably be a problem for developers.