Broken rendering when removing elements about react-masonry-component HOT 15 CLOSED

v5.0.1 released

from react-masonry-component.

I am running the following without any console errors:

• react-masonry-component 5.0.0
• react 15.4.1
• masonry-layout 4.1.1 (transitively)
• outlayer 2.1.0 (transitively)

Though I think we've figured out in #63 why there is no crash in my code...the fact that I don't use transitions and both 5.0.0 crash bug repro cases do.

However (and this is troubling), if I roll back to 4.4.0, I am unable to reproduce my old broken behavior that motivated this change. :( Likewise, I am as-of-yet unable to generate a 4.4.0 webpackbin that demonstrations the problem.

I have to head to sleep now, but I will investigate more deeply tomorrow. Might be a webpack cache issue confusing me late-night, or maybe jumping back through my commit log will help me reproduce it and isolate what's going on.

(And hopefully once we dig to the bottom of this, we can then also fix some of the tricky namings of these variables that confused me.)

from react-masonry-component.

@eiriklv do you know why it was done this way originally? This code has been here from the beginning.

from react-masonry-component.

I seem to recall that this was done because of how React worked internally, which may very well have changed. All of the diffing stuff is done to avoid a complete re-layout and to be able to leverage the different methods that Masonry has.

The oldChildren should contain only the elements that remain from the last render.

Let me see what I can dig up from the back of my mind 😅

from react-masonry-component.

From looking at the code (not able to test it atm), I can't really see why I did it this way. It's either just plain wrong from my part, or I did it for some obscure reason which I do not remember. The solution proposed by @mikelambert should work (thanks for poking!). And if it does in practice, then all is good in the hood 👌 Submit a PR 👍

Merry Christmas everyone 🎄🎅😌

from react-masonry-component.

Thanks for the quick investigation, on Christmas Eve no less!

from react-masonry-component.

fixed in v5.0.0

from react-masonry-component.

Bummer - this change causes layout problems and a crash in 'componentDidUpdate' when I change the entire set of children of the component without unmounting it first.
Relates to #63
Created a repro - see the linked issue.
Also @eiriklv said:

The oldChildren should contain only the elements that remain from the last render.

And after this change I assume @mikelambert gets 20 oldChildren - which is wrong.

@mikelambert Can you post your code that uses the Masonry component? I suspect something funky with the keys you assign to the children array..

from react-masonry-component.

Sorry about the breakage/regression. :(

After this change, I was getting 20 oldChildren (as stated in my original post as my goal....because doing so also made removed work too). I guess I'm unclear and had a different interpretation of eiriklv's statement: "All twenty elements remain from the previous render (in the DOM), and then ten need to be removed from it."

Namely:

        var removed = oldChildren.filter(function(oldChild) {
            return !~newChildren.indexOf(oldChild);
        });

With your interpretation, if the oldChildren only contains elements that remain (and are preserved into the next render)...then it's impossible to find any oldChildren that would not be in newChildren right? And removed would always be empty? This seems like a strange way to write code, if that interpretation is true...

As far as my code:

    const tutorialComponents = filteredTutorials.map((tutorial, index) =>
      <Tutorial key={tutorial.getId()} ... />);
    return <Masonry>{tutorialComponents}</Masonry>;

where getId() returns an string matching /[a-z-/]+/, and there are no duplicate keys in the array. You can see it in action (with the React debugger, if you have that installed) at http://www.dancedeets.com/tutorials

from react-masonry-component.

No worries, we will figure it out eventually. I currently reverted back to 4.4.0 until I know how to best proceed.

Seems like you use the same approach for the keys as I do - so it doesn't make sense to me how you take children which are no longer attached to the parent in the dom, send them to "remove" of Outlayer and do not crash with the same error.
Only explanation I could think of is something different in how the react rendering works. I use react 15.4.1 at the moment. And you?

Looking at the code I think the naming could be better:
this.domChildren -> this.currentlyKnownDomChildren
getNewDomChildren() -> getCurrentChildrenFromDom()

Since getNewDomChildren just queries the current children list on the dom:

var node = this.refs[refName];
var children = this.props.options.itemSelector ? node.querySelectorAll(this.props.options.itemSelector) : node.children;
return Array.prototype.slice.call(children);

And does not have a clue which is a "new" and which is an "old" child..

In light of this, of course there could be some 'oldChildren' not in 'newChildren' since the dom may not have some of the 'oldChildren' now that react removed them. This is exactly what happens in this case.

Makes sense?

Anyway, Could you create a webpack bin with 4.4.0 and a repro of the original problem you intended to solve? I will dig into that and maybe understand how you get away with the existing change.

from react-masonry-component.

Then there was a reason after all (which I could not remember previously).

Unfortunately the projects I initially created this for has been dormant for years, so my intimate knowledge of the inner workings isn't as fresh anymore 😅.

If I had more time I would help out more, so thank you so much for poking and fixing guys!

I also agree that the naming can be vastly improved 👍 That would probably make the intention of that piece of code a lot more clear..

from react-masonry-component.

Alright, I have built a repro for my original bug: http://www.webpackbin.com/4kUsSX6NG

Seems that you used numerically-increasing keys (as did my initial failed attempts on webpackbin), which meant that removing items went from keys 0-to-9 to keys 0-to-4 (ie, there were only keys removed at the end of the list).

In my case, I'd prefer to re-use my components, and so am specifying a stable key. So when removing keys 0-4, I am left with keys 5-9: ie, the first 5 keys were removed from the list, which breaks layout.

In my repro, I've added a break/fix button that demonstrates enumerated keys (fixed) vs my stable keys (broken). Just toggle items twice in the 'broken' scenario, and you should see a left-aligned sequence of items that don't masonry-layout across the entire container (ie, the bug)

from react-masonry-component.

This is an awesome bug. No doubt. Here are my findings.

Whenever the first-already-rendered item is removed from the dom by react - the breakage happens. (all boxes stacked on a single column)
As long as the first item remains and the others are added/removed - all is good, regardless if the detached elements are not removed from Masonry itself.

Why is that?

Looking at the "layout" code in masonry, it can be seen how the "coulmn width" is calculated by default:
here

   // if columnWidth is 0, default to outerWidth of first item
    if ( !this.columnWidth ) {
      var firstItem = this.items[0];
      var firstItemElem = firstItem && firstItem.element;
      // columnWidth fall back to item of first element
      this.columnWidth = firstItemElem && getSize( firstItemElem ).outerWidth ||
        // if first elem has no width, default to size of container
        this.containerWidth;
    }

So the first item's width is the fallback when there is no other method specifying the columnWidth. (like using the options). Clearly when the first item exists in masonry but does not exist in the dom, its width would be 0. and because:

// if first elem has no width, default to size of container

The column width equals the container width (which is not specified probably in your case) - hence all items are stacked on a single column.

If I would set the columnWidth option either fixed or using a selector - that would be the calculation method and the first item could peacefully retire from the dom. From Masonr'y documentation:

We recommend setting columnWidth. If columnWidth is not set, Masonry will use the outer width of the first item.

Here is a modified version with your repro using the 'selector' approach for column sizing.
http://www.webpackbin.com/NJcpr66Ef
The problem does not occur.

To summarize:

• in version 4.4.0 if items are only removed by react from the dom they are not removed from masonry.
• As soon as at least one other item is added (which was not there before) they are removed from masonry since 'reloadItems' is called.
  See this repro as an example: http://www.webpackbin.com/NJwsuT6Nf
• When the first item is removed (=leaked) and the default column width calculation method is used - the column width would be the container width

So your problem occurs because you had the following combination:

• 4.4.0
• first item removed
• only removed items, did not add any
• did not specify a way to calculate column width
  Here is the absolute minimum required to repro http://www.webpackbin.com/EkksY6aNf

I suggest:

• reverting the change - which causes a potential crash in Outlayer
• checking if any item removed -> forcing a "reloadItems"

That would make sure that when we know react and masonry are fighting over who is owning the dom - we would just tell masonry - get all the items again, your internal state is not trustworthy.

#64 - I have created two tests that pinpoint what I describe above and a fix I think it reasonable given all the chaos in the diffing algorithm.

from react-masonry-component.

Thanks for looking into this! I had to roll back to 4.4.0 as the layout is broken in 5.0.0 as you guys discovered.

I'm using react 15.4.1 as well.

from react-masonry-component.

Sorry to hear - Hopefully a fix is on the way.
You can also set the transition time to anything above 0 (even 1 ms) to work around the problem with 5.0.0.

from react-masonry-component.