Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lazy loading is useless right now #453

Closed
1 task done
Bobgy opened this issue Aug 8, 2018 · 22 comments · Fixed by #462
Closed
1 task done

lazy loading is useless right now #453

Bobgy opened this issue Aug 8, 2018 · 22 comments · Fixed by #462
Labels

Comments

@Bobgy
Copy link
Contributor

Bobgy commented Aug 8, 2018

Lazy loading should make mounting the swipeable views component faster by only mounting not initially visible views later, but how lazy loading is implemented right now is that, on initial render only view of the chosen index is rendered -- this is correct --, but on componentDidMount event, it sets the firstRender state to false, which triggers a synchronous re-render of this component. In this re-render, all the other views are mounted too. This makes lazy-loading completely useless, because in the end, all the views are rendered synchronously.

reference: https://reactjs.org/docs/react-component.html#componentdidmount

You may call setState() immediately in componentDidMount(). It will trigger an extra rendering, but it will happen before the browser updates the screen. This guarantees that even though the render() will be called twice in this case, the user won’t see the intermediate state. Use this pattern with caution because it often causes performance issues. In most cases, you should be able to assign the initial state in the constructor() instead. It can, however, be necessary for cases like modals and tooltips when you need to measure a DOM node before rendering something that depends on its size or position.

  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

views not initially visible should be mounted asynchronously with either raf or setTimeout.

Current Behavior

views not initially visible are still mounted synchronously after componentDidMount event.

Steps to Reproduce (for bugs)

This can be observed in demo.

  1. go to demo page, https://n075p8mq7l.codesandbox.io/
  2. open chrome dev tools
  3. click "start profiling and reload page"
  4. check the user timing section and see that "Lifecycle hook scheduled a cascading update", and all the views are mounted in the cascading update synchronously.

Context

When you put something huge into the views, performance is not acceptable without lazy loading.

Your Environment

Tech Version
react-swipeable-views 0.12.16
React 16.4.1
platform macOS
etc
@Bobgy
Copy link
Contributor Author

Bobgy commented Aug 8, 2018

@oliviertassinari Can I get some explanations?

@oliviertassinari
Copy link
Owner

@Bobgy You are right. Thank you for reporting the issue. Right now, the lazy loading only works with server side rendering. Using a setTimeout sounds like a great idea!

@gaearon
Copy link

gaearon commented Aug 10, 2018

We're going to have a first-class API for mounting invisible components asynchronously without blocking the thread in the future, right inside React. Stay tuned :-)

@Bobgy Bobgy mentioned this issue Sep 2, 2018
@Bobgy
Copy link
Contributor Author

Bobgy commented Sep 2, 2018

@gaearon I'm happy to hear that, it will make this implementation a lot simpler.
Just curious about some details.
I think a common tricky case is that:

  1. a component with some heavy invisible children is rendered and has an onMount animation
  2. invisible children rendered asynchronously, but still right after, then it could cause a jank because that's slow

Is the new first-class API able to handle that (particularly, mount heavy invisible children at the correct timing without causing jank)? just guessing probably with some requestIdleCallback magic?

@gaearon
Copy link

gaearon commented Sep 2, 2018

Normally React wants to keep the UI consistent with what you told it to render.
So if you render

function Slideshow() {
  return (
    <div>
      <Expensive1 />
      <Expensive2 />
      <Expensive3 />
    </div>
  );
}

then it will have to call render methods and create DOM nodes for the whole trees of Expensive1, Expensive2, and Expensive3, when Slideshow is being mounted.

However, maybe you're only showing Expensive1 on the first render, and Expensive2 and Expensive3 are not immediately visible. Like if this is really a side show.

Today, it's idiomatic to render just the current node, e.g.

function Slideshow(props) {
  return (
    <div>
      {props.current === 0 && <Expensive1 />}
      {props.current === 1 && <Expensive2 />}
      {props.current === 2 && <Expensive3 />}
    </div>
  );
}

The upside of this is that now your first render of <Slideshow current={0} /> only includes Expensive1. However, the downside is that the moment you switch to <Slideshow current={1} />, you will experience jank from creating and rendering the whole Expensive2 tree.

Ideally what we want to do is to tell React that when we render <Slideshow current={0} />, we want to show Expensive1, but we want to start preparing Expensive2 when the browser is idle. This way it won't block the initial render or cause jank, but by the time you click "next" Expensive2 might just already be complete, and in this case it'll just replace the DOM node.

This is exactly what time slicing which I partially demoed in my talk will allow. Your code might look something like:

function Slideshow(props) {
  return (
    <div>
      <div hidden={props.current !== 0}>
        <Expensive1 />
      </div>
      <div hidden={props.current !== 1}>
        <Expensive2 />
      </div>
      <div hidden={props.current !== 2}>
        <Expensive3 />
      </div>
    </div>
  );
}

Note that hidden is a real HTML attribute. (It acts similar to display: none.) But it could also serve as a hint to React that the tree inside it doesn't actually need to be committed immediately — because it's not visible anyway. Also note: this is not a final API, I’m just explaining what it lets you do.

So when you render <Slideshow current={0} />, React mounts

<div>
  <div hidden={false}>
    <Expensive1 />
  </div>
  <div hidden={true}>
    <!-- not ready yet -->
  </div>
  <div hidden={true}>
    <!-- not ready yet -->
  </div>
</div>

and whenever the browser is idle (e.g. when you look at the first slideshow item), it will start working on Expensive2 in small chunks. When it's ready, it will just append it to the hidden div — which won't be observable to the user because it's hidden.

<div>
  <div hidden={false}>
    <Expensive1 />
  </div>
  <div hidden={true}>
    <!-- not visible, but prepared during idle periods and now ready! -->
    <Expensive2 />
  </div>
  <div hidden={true}>
    <!-- not ready yet, but React will start working on it next -->
  </div>
</div>

Now if you switch to <Slideshow current={1} />, React doesn't need to do any extra rendering because it has already "prepared" Expensive2 ahead of time.

And if you switch too fast, and React hasn't been able to fully prepare Expensive2 yet, it will just pick it up where it left off but set a much more aggressive deadline since we want to see results within ~150ms.

I hope this explains it a bit! The crucial part here is time slicing. This optimization wouldn't make sense if pre-rendering Expensive2 or Expensive3 blocked the thread since in this case it wouldn't be worth slowing down the interactions while Expensive1 is visible. But thanks to React's architecture, we can actually start pre-rendering Expensive2 and Expensive3 in small chunks without blocking the thread, and that's what will enable this optimization.

@demiacle
Copy link

demiacle commented Sep 2, 2018

cool beans, but doesn't this break instances where a component gets initial state from a prop and is rendered before they are expected?

@gaearon
Copy link

gaearon commented Sep 2, 2018

@demiacle I don't understand the question. Can you explain in more detail?

@demiacle
Copy link

demiacle commented Sep 2, 2018

Hmm just double checked for sanity, It's discouraged in the docs on constructor so its probably an anti pattern, but it does have a use case. I was playing around with it here. Essentially with your proposed solution, clicking the button wouldn't change any text because the state was already derived from the props.

@rbutera
Copy link

rbutera commented Sep 2, 2018

Will this change the way we approach route/page transitions in React?

@gaearon
Copy link

gaearon commented Sep 2, 2018

@demiacle If the "use case" you're referring to is capturing props at the time constructor was called then yes, you might capture them earlier than intended if you use pre-rendering. I'm struggling to understand why you'd want a component API like this since it's pretty unintuitive that changing a prop has no effect later. This is why we discourage it. I wouldn't say pre-rendering would "break" this — it's just that you need to decide whether you want pre-rendering or you want this behavior (which is fundamentally incompatible with the idea of pre-rendering).

@Raigasm It might (note you won't have to use it but might find it beneficial). We'll see.

@rbutera
Copy link

rbutera commented Sep 2, 2018

What I'm wondering is if this has applications beyond the case where all the components are expensive.

a la your example:

<div>
  <div hidden={false}>
    <Inexpensive /> 
  </div>
  <div hidden={true}>
    <Expensive1 />
  </div>
  <div hidden={true}>
    <Expensive2 />
  </div>
</div>

@demiacle
Copy link

demiacle commented Sep 2, 2018

With programming its never about the why though haha, if it can happen it will. I was only interested in being explicit because in the example I presented, there are 2 identical components that are rendered differently. Just thought it should be noted. Anyways super cool stuff!

@gaearon
Copy link

gaearon commented Sep 2, 2018

What I'm wondering is if this has applications beyond the case where all the components are expensive.

I'm using the word "expensive" to better get the point across. It doesn't have to literally be expensive — no matter how fast your rendering is, it would still be faster to do it earlier in an idle period (and then switch instantly).

@dennis-8
Copy link

dennis-8 commented Sep 2, 2018

Dan's example reminded me of tabs and how the content of active one is shown only...
But still the content off all tabs is there somewhere.

"Ты суслика видишь? И я не вижу. А он есть"

@dcollien
Copy link

dcollien commented Sep 3, 2018

This is very cool. Purely speculating here without a solid use case: would there be a way to specify rendering priority? As per the above description it seems like the background rendering is perhaps prioritised by the order in which components are specified, or just spread across all equally in some piecemeal way, but I imagine that it may be the case in a more complex layout that one hidden component that is definitely going to be the next thing the user sees should be prioritised over some other hidden component which may have lesser importance (e.g. less important for it to be jankless, or it is suspected to be more likely to come into play later, or perhaps very few people even reveal that component, etc)

I can imagine this being useful when you have a primary user pathway which can be optimised to have the best jank-free experience, when there exists less important, but potentially very expensive to render hidden components off to the side

@gaearon
Copy link

gaearon commented Sep 3, 2018

It's pretty early to see right now. We'll see how it evolves after we use it for a while.

@vladnicula
Copy link

@demiacle I was thinking at the same thing as you. With this api we could have both benefits if we split the component that needs the initial state from a prop into a wrapper that just computes/stores this state and a child that is used inside a hidden div.

This would help me with a few "render only when visible" cases where content is not reliant on any API data.

class ExpensiveOnDemand extends React.Component {
  constructor (props) {
     super(props);
    // do stuff with state and initial props
    this.state = { ... };
  }
  render (props) {
    return (
        <div hidden={!this.props.visible}>
            <ExpensiveStuff {...this.props} {...this.state}/>
            // or maybe something with this.props.children
        </div>
    );
  }
}

This will probably need more work for sub components that fetch data on componentDidMount, which has been a common practice for me when rendering tabs on demand.

@ChrisLincoln
Copy link

In our app, we generally would not have rendered Expensive2 or 3. Should we be adopting a technique where they are hidden, for instance with DOM or CSS attributes?

@josgraha
Copy link

josgraha commented Sep 3, 2018

@gaearon is there a plan for an opt-in component like <React.Prerender> that can optimize parts of the tree that want to coerce prerendering and opt-out for everything else? Do you even see a use case for that given the hidden attribute (or whatever prop you finally go with) already does this implicitly?

@gaearon
Copy link

gaearon commented Sep 3, 2018

@ChrisLincoln No, my explanation is about a future feature of React. It doesn't exist today and won't work for you the way I described.

@josgraha I don't know, we'll think more about this before finalizing the API.

@xialvjun
Copy link

xialvjun commented Sep 10, 2018

@gaearon how about the name defer which is more general than hidden?

<div>
  {list.map((item, index) => (
    <div key={item.id} defer={index !== current_index}>{item.content}</div>
  ))}
</div>

@studevsdev
Copy link

studevsdev commented Sep 19, 2018

I am using swipeable to swipe gvis charts. The json data for each chart I fetch from my S3 API each time a swipe occurs. To solve the problem when it's not ready I have a loading spinner like this:

<SwipeableViews enableMouseEvents bindKeyboard index={index} onChangeIndex={this.handleChangeIndex}>

{
  this.state.dataLoadingStatus === 'ready' ? (
    <div className="Container">
      <Chart
        chartType="LineChart"
        width="100%"
        height="400px"
        data={this.state.message}
        options={options}
      />{index}
      <br></br>
      {this.state.urlVar}
    </div>
  ) : (
    <div style={style}>
      <img src={loading} alt="loading"/>
    </div>
      )
}
   </SwipeableViews>

It does not fetch all the json at the same time from my API, but it renders the chart component for all items . So it seems to work. I will see how 230 items work or if it's get to slow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.