eiriklv / react-masonry-component

A React.js component for using @desandro's Masonry
MIT License
1.44k stars 145 forks source link

Browser crashs when re-rendering Masonry childrens #85

Open sandrina-p opened 7 years ago

sandrina-p commented 7 years ago

When updating the Masonry items (childrens), the browser crash.

You can view it live here

Reproduce: Switch between tabs (woman and man), or select/unselect one of the carousel items (the movies). The Masonry, for some reason breaks and the browser crashs.

This is the code that renders the Masonry:

import Masonry from 'react-masonry-component';

// ...

const posters = this.props.content.map((item, index) => {
      return (
        <OutfitPoster
          key={index}
          className={[styles.productItem, stylesMasonry.item].join(' ')}
          title={item.content_title}
          description={item.product.name}
          imageSrc={item.content.thumbnail}
          shopLink={item.product.link}
        />
      )
});

return (
   <Masonry className={[styles.products, stylesMasonry.masonry].join(' ')}
        updateOnEachImageLoad
      >
          {posters}
      </Masonry>
    );
)

This this.props.content changes each time I filter it by selecting a tab or movie.

Thanks!

afram commented 7 years ago

It's kind of hard to see without access to the codebase, but I suspect it's a few things.

1) There are many elements on the page, especially images. There's no need to load all 180+ images in the carousel on page load. You could consider lazy loading images as they are needed. You have over 800 elements on the page.

2) The issue seems to happen when I click 'tab 2 -> tab 1 -> tab2'. On that third click it freezes every time. What happens on category change? take a look at the logic there because it looks like something kicks off that takes control of the JavaScript thread. Do you have some kind of an infinite loop happening there?

Does this still happen when you load less images on the page? (I'm guess it probably will) Try and use the dev tools to trace the execution path and see where your code is spending most of time. You may find some clues.

sandrina-p commented 7 years ago

Thanks for the help. I removed the carousel and the issue still happens... if I remove Masonry, it doesn't crush. When clicking on a tab, it only changes the content to map. I think the issue is when the masonry elements' key match the element content twice. (which is what happens when you select back the tab 1)... 🤔

sandrina-p commented 7 years ago

Hey there, I found a solution! 🎉

For some reason, React gets confused when Masonry changes its content and end up at some point with a "repeated" component (this case, switching back to tab).

So I tried several things, such as creating a new array to loop the childrens or changing its key at each render. Nothing worked.

Then I just added a "dynamic" key to <Masonry> component and voilà!

<Masonry key={new Date().getTime()} updateOnEachImageLoad>
    {posters}
</Masonry>

For being a different key at each render, React somehow completely rebuild the component. I'm not sure how/what happens under the wood, but the true is that works !

You can check it on the original link I shared before. It will still be on for some time.

Thanks for the help anyway!

afram commented 7 years ago

Glad you found a solution.

That sounds a little odd, it makes me suspicious that maybe React Masonry Component is doing something it shouldn't (or not doing something it should).

You're the first person to raise this issue, so thanks for giving me something to look into ;-)

afram commented 7 years ago

Hi @sandrina-p

I built a simpler version of what you have and it works fine in the example: https://www.webpackbin.com/bins/-KnBuUxuxu9B16Cr1ED5

I wonder if maybe there's something else going on in your code that's causing the issue? Forcing re-render with a different key is possibly just masking the underlying issue.

sandrina-p commented 7 years ago

Hey @afram

Sorry the late answer. I have a very similar code to yours, and it crushs. I don't use forceUpdate() on my project. I have more stuff on each item, such as react-truncate... It's strange. When I pick up the project again, I'll check it again with more detail.

Thanks anyway :)

jerryslaw commented 7 years ago

Hi. I have similar issue. onLayoutComplete is calling several times, I don't know why. When I use @sandrina-p solution, it fires only once, but - I want to change my redux state when layout is completed, so I'm calling some function from props, and then.... My call stack is exceeded, onLayoutComplete is in infinite call loop.

My code is:

import React from 'react';
import PropTypes from 'prop-types';
import Masonry from 'react-masonry-component';
import { connect } from 'react-redux';
import compose from 'lodash/fp/compose';

import FetchContainer from 'containers/Fetch';
import { galleryFetch, openGalleryPreview } from 'store/actions';

import './Gallery.scss';

const masonryOptions = {
  itemSelector: '.gallery__item',
  columnWidth: 300
};

class GalleryComponent extends React.Component {

  static propTypes = {
    data: PropTypes.arrayOf(PropTypes.object),
    openPreview: PropTypes.func,
    changeLayoutStatus: PropTypes.func
  };

  static defaultProps = {
    data: []
  };

  componentDidMount() {
    this.props.changeLayoutStatus(false);
  }

  layoutComplete = () => {
    this.props.changeLayoutStatus(true);
  };

  render() {
    const { data: images, openPreview, changeLayoutStatus } = this.props;

    return (
      <div className="gallery">
        <div className="gallery__images">
          <Masonry
            key={new Date().getTime()}
            className="gallery__masonry"
            options={masonryOptions}
            onLayoutComplete={this.layoutComplete}
          >
            {
              images.map((image, i) => (
                <div key={i} className="gallery__item">
                  <img src={image.url} alt={image.name} onClick={() => openPreview(images.indexOf(image))} />
                </div>
              ))
            }
          </Masonry>
        </div>
      </div>
    );
  }
}

const mapDispatchToProps = {
  changeLayoutStatus: changeGalleryLayoutStatus,
  openPreview: openGalleryPreview
};

export default compose(
  FetchContainer(galleryFetch, 'gallery'),
  connect(null, mapDispatchToProps)
)(GalleryComponent);

Difference is that () => console.log('called') is calling only once (as expected), but my function is calling infinitely.

PS. My GalleryComponent is not re-rendering after each call of changeLayoutStatus

afram commented 6 years ago

@jerryslaw I know this is a very old ticket. Are you in a position to try the latest version and see how it works for you. Does it solve your problem?