trufflesuite / drizzle-react-legacy

176 stars 48 forks source link

using <DrizzleContext.Consumer> re-renders the component infinitely #51

Closed kombos closed 5 years ago

kombos commented 5 years ago

I'm trying to build a simple dapp using drizzle-react's 'DrizzleContext'. This is how my index.js file looks like:

import React from 'react';
import ReactDOM from 'react-dom';
import { Drizzle, generateStore } from "drizzle";
import { DrizzleContext } from "drizzle-react";
import './index.css';
import App from './App';
//import * as serviceWorker from './serviceWorker';
import drizzleOptions from './drizzleOptions';

const drizzleStore = generateStore(drizzleOptions);
const drizzle = new Drizzle(drizzleOptions, drizzleStore);

ReactDOM.render(
    (<DrizzleContext.Provider drizzle={drizzle} >
        <App />
    </DrizzleContext.Provider>), document.getElementById('root'));

Also, this is how my < App / > component looks like:

import React from "react";
import { DrizzleContext } from "drizzle-react";

    export default () => (
        <DrizzleContext.Consumer>
            {drizzleContext => {
                const { drizzle, drizzleState, initialized } = drizzleContext;

                if (!initialized) {
                    return "Loading...";
                }

                console.log("value of initialized: ");
                console.log(initialized);

                return (
                    <div>hello world</div>
                );
            }}
        </DrizzleContext.Consumer>
    )

turns out that using < DrizzleContext.Consumer > tag starts re-rendering the complete state infinitely, as is evident in the console log where all the mentioned logs keep repeating. Is this normal behavior ? Is there a way we can arrest this infinite re-render inside a Functional Component ?

adrianmcli commented 5 years ago

Have a look at this part of the README: https://github.com/trufflesuite/drizzle-react#preventing-unnecessary-re-renders-optional

Garoth commented 5 years ago

Since @kombos pinged me specifically, I'd like to say that this is indeed a big pain point. cc @adrianmcli

I know how to mitigate the "event spam" using PureComponents and other tricks, but the reality is that this is a cat and mouse game that tends to introduce bugs. Almost all of my components are PureComponents, but that's not enough to fully avoid unnecessary renders. I end up having to write special code using internal state, instance variables, timers, etc -- to specifically prevent rendering if I've analyzed the data and think it's basically the same.

Even still, my site (https://flowerpatch.app) often runs into weird edge cases. For example, I have found that after people make store purchases, the transaction cache starts getting really spammy in a hard to control way. As people make more purchases and listen to more transaction events, my website gets choppy and the animations start resetting due to re-renders.

Also, the lack of Promises support in Drizzle makes it really REALLY hard to write performant blockchain code which has dependencies. For example, if you want to iterate over a contract array, you need to fetch the length of the array, then each of the elements one at a time. These variables are dependent on one another, and drizzle makes it very hard to do one call and then the next one. I often end up using normal web3 calls with promises in these cases, or create complex react component structures to avoid the problem.

adrianmcli commented 5 years ago

@Garoth

To give you a short answer, you are right. It sounds like Drizzle is just not architected to suit your needs. But we are actually working on Drizzle-Utils that will enable most of your use-cases. Stay tuned for that.

In the mean time, I have some questions:

I end up having to write special code using internal state, instance variables, timers, etc -- to specifically prevent rendering if I've analyzed the data and think it's basically the same.

Why is there a need for timers? Would your problem be solved by using the old context API with the special drizzleConnect?

As people make more purchases and listen to more transaction events, my website gets choppy and the animations start resetting due to re-renders.

How are you listening to transaction events? What do you mean by the "the transaction cache starts getting really spammy"? I'm not sure I understand what you mean here.

the lack of Promises support in Drizzle makes it really REALLY hard to write performant blockchain code which has dependencies

Since state mutating transactions are one-off, it may not be a good idea to use Drizzle for it. Using the raw Web3.js API will probably make your life easier. The biggest benefit to Drizzle I see is reactively reading from a constant function.

Garoth commented 5 years ago

@adrianmcli

Timers: There is often a need to process the incoming props in render, and then schedule some kind of state change. This is a react anti-pattern, but works quite well if you slap a timeout of 0 on it, so it gets scheduled for the next render loop. Fyi, I do this in render because PureComponent prevents usage of getDerivedStateFromProps, and the other APIs like willReceiveProps and some of the render lifecycle has been deprecated. Also, the fact that getDerivedStateFromProps is static is another limiting factor

Legacy drizzleConnect: I did implement that first, and I found it to be somewhat buggier. I moved to the Context API to get more fine grained control and to stay up to date with React. drizzleConnect did do a better job of filtering duplicate data, but iirc there was some need to move to Context to get latest features or something.

Transaction Event Spam: I haven't fully debugged this yet, since it's something I just started to notice. Basically, I have some various components that listen for drizzleState.transactions and the transaction stack. I'm sure I can implement some event filtering there to reduce re-renders caused by this, but I'm not exactly sure what's going on. It's a hard to find and debug class of issues. My guess is that one of the transactions type caches often changes its root memory reference, and causes PureComponents to re-render.

Promises: I politely disagree with your assessment. I think it would be very possible for Drizzle to support both the current mode, and also optionally offer promises that just work to tell you when your data is ready. Then I could write my array example -- tell drizzle to watch the array length. Then when it gets or changes the length, it fires up the promise or callback function. From there, I can iterate the length and tell drizzle to watch the particular elements.

This would be a substantial benefit because:

  1. I wouldn't need to write my own caching logic,
  2. I wouldn't need to write code in render that waits for drizzle data to come in and then do the second step.

That said, as I hinted before, I've found a trick to "work around this" in a sensible way. I create component structures such that the parent component just watches the length and creates a variable amount of sub-components based on the length. Each of the sub-components then receives its "slot number" in the array, and fetches its own data. With this design, Drizzle works quite well -- but it's pretty hard to realize what to do, and there are a fair bit of limitations with this approach

I'd be happy to further discuss my Drizzle experience with the Truffle team. Let me know if you guys wanna do a call or hook up online somehow

adrianmcli commented 5 years ago

I think it would be very possible for Drizzle to support both the current mode, and also optionally offer promises that just work to tell you when your data is ready.

I think this is going to be quite difficult because you'd have to interrupt the saga that is currently written into Drizzle, but I could be wrong. That being said, if you feel strongly about it, I would advise you to submit a PR or at least a prototype implementation.

As for the other issues, I can't comment on that because that is very specific to how you have decided to structure your app. We are planning on doing some user interviews and surveys soon, and if you'd like to be on that list, send me an email @ adrian@trufflesuite.com.

At any rate, thanks for taking the time to write up your thoughts.

kombos commented 5 years ago

First of all thanks for responding both of you. I'm still learning Drizzle and would like to clarify a doubt: in the example code i posted above, would using PureComponent help in any way, since we are deriving the whole context in < App /> component. Hence, even if we re-write this component (< App / > ) as a class PureComponent, wouldn't it keep re-rendering since the PureComponent only tracks changes to 'state' or 'props' and not 'context' ?

Also, won't the same apply to the child components ? as long as the child components are getting their values as 'props' from parent component, the re-render can be arrested using PureComponent, but if any child component accesses data using 'context', then it starts re-rendering infinitely again. to not use 'context' would be defeating the whole purpose of the new Context API.

adrianmcli commented 5 years ago

@kombos The function immediately inside of <DrizzleContext.Consumer> is going to get called on every block poll. There's no getting around that, but in that function you will return a component to be rendered (let's call this the <InnerComponent>).

It is inside the <InnerComponent> where you will need to specify your logic on whether or not to render. One way to do that is to make <InnerComponent> extend from PureComponent like the docs suggest.

kombos commented 5 years ago

could you please guide me as to how to factor the components in this scenario : I have the following two items in my contract :

mapping (uint256 => struct) public structVals;
uint256[] structKeys;

function getKeys() {
    return (structKeys);
}

now, in my dapp, I need to first receive the keys and then for each key, I have to render all the members of struct. so , assuming i have my dapp structured as mentioned in the above example, which is :

index.js ------(DrizzleContext.Provider) -----> App.js -------DrizzleContext.Consumer ------> DrizzleApp

inside < DrizzleApp / > , i'm running a cacheCall() to get the keys, and then forwarding each key value to another component < Struct / > using array.map. Now, inside < Struct / > I have to again do cacheCall() to get the member values of struct, referenced by the key. As I understand, I won't be able to declare either of DrizzleApp or Struct component as PureComponent as I'm using cacheCall() inside both. Correct me if i'm wrong? Whats the best practice to implement this ? Tx in advance.

kombos commented 5 years ago

the problem i'm facing specifically is this: inside a component (DrizzleApp), even after the dataKey is received/generated, it still takes couple of iteration of renders for the return value from the call to a constant function to be populated. Because of this, if the component (DrizzleApp) is declared PureComponent, the values are not getting rendered as it stops re-rendering before the return value is populated (and after the dataKey is received/generated).

There has to be a standard approach to this...

adrianmcli commented 5 years ago

Yeah that is true, the list itself will likely re-render every time, but each list item can maintain its own state. This is all super tricky because of Solidity's inability to return a list of structs.

In most cases, if it's not causing performance problems, I think it's completely fine to let it re-render multiple times. But keep in mind that you should keep your Drizzle logic separate from your pure rendered components. You should always have a container component that does this fetching logic, and an actual visual component that takes these values as props, which can be a PureComponent.

kombos commented 5 years ago

appreciate all the responses . I'm closing this for now. Tx.