Network-aware LoadingContainer

[SeanJCasey]

Uses the networkMismatch flag to allow handling of network-related halting of loading drizzle.

Updates the redux LoadingContainer and adds a new Context API-friendly Loading Container

[cds-amal]

@SeanJCasey, when you have time, please update the README and the test-apps to reflect/document this feature. I am excited to try this out! Thanks!

[SeanJCasey]

Update the test app for the react context api with a LoadingContainer. We can add the overrides for this component at a later time. The README reflects the new props for this component.

[adrianmcli]

@SeanJCasey I'm having trouble running this on the test-UI apps. I have:

1. Pulled the branch,
2. Ran lerna bootstrap,
3. And put the app on a network where the contracts are not deployed.

I'm getting the following error on both React apps (legacy context AND new context) instead of the nice warning message:

[SeanJCasey]

@adrianmcli Did you set networkWhitelist in drizzleOptions? If the option is absent, it will default to [], which means all networks are approved.

[adrianmcli]

@SeanJCasey Can you add that to the test-UI app then?

[SeanJCasey]

@adrianmcli Sure, I can add a networkWhitelist config option with values if that's what you mean. So long as you're sure it won't trip people up to have particular networks blocked. What networks do you want to be enabled?

[adrianmcli]

@SeanJCasey I think the goal is to make sure that the repo's test apps are actually able to test the features in our packages. Do you have any thoughts on what the defaults should be?

[SeanJCasey]

@adrianmcli How about [1, 4] (Mainnet, Rinkeby) for the allowed networks? Then Ropsten etc will catch the network mismatch. Ganache is allowed by default, regardless of the values in the array.

[adrianmcli]

@adrianmcli Did you set networkWhitelist in drizzleOptions? If the option is absent, it will default to [], which means all networks are approved.

Actually I'm confused. Why is a network whitelist needed? I thought the purpose of this PR is to make it such that the dapp will try to detect the contract on the current network, and if it isn't there then it'll show a friendly message in the UI.

[SeanJCasey]

The purpose of this PR is to allow the LoadingContainer to work with the networkWhitelist option that was merged previously. This blog post details and has links to both that PR and this one: http://seanjcasey.com/blog/drizzle-network-whitelist

Good question re: explicitly defining a networkWhitelist rather than reading from contracts or catching an error. @cds-amal and I discussed this and decided on the current option so that devs can be explicit about which networks they do or don't want their dApps accessed on and which are expected to work.

[adrianmcli]

Right, but if it doesn't support it, it should display a message instead of throwing an error in the console right?

[SeanJCasey]

Yes, but the idea was to separate out functionality and UI/UX. Rather than assuming UI/UX, we just provide a developer with the networkMismatch flag that is produced when the provider is connected to a network that is not in the networkWhitelist. LoadingContainer then serves as a sample UI implementation.

[adrianmcli]

@SeanJCasey Ok sounds good. So let's just put in the [1, 4] and let's see where we go.

[SeanJCasey]

@adrianmcli I added the networkWhitelist to react redux and context test uis and squashed into the feat commit. Tested those two flows. We should be good to go!

[adrianmcli]

Thanks @SeanJCasey, I'm on vacation until Tuesday unfortunately but let's get this merged asap.

[cds-amal]

Just adding a note that this resolves #23

[adrianmcli]

Seems to work as expected! Thanks @SeanJCasey

[adrianmcli]

I have some questions regarding the code and refactor requests.

[adrianmcli]

This section can be re-written as:

return this.props.networkMismatchComp || <main>...</main>

This allows us to avoid the nested if statement and also makes it clear that this.props.networkMismatchComp is a component.

[adrianmcli]

What is the purpose of this comment block?

[adrianmcli]

This component has a ton of if-statements, consider refactoring.

[adrianmcli]

As above, this can be written as:

return this.props.errorComp || <main>...</main>

In order to avoid another if-statement.

[adrianmcli]

Same as above.

[adrianmcli]

Consider de-structuring and using intermediate variables.

[adrianmcli]

@SeanJCasey I've went ahead and refactored LoadingContainer.js:

import React, { Children, Component } from "react";
import PropTypes from "prop-types";

class LoadingContainer extends Component {
  renderFailed() {
    return (
      this.props.errorComp || (
        <main className="container loading-screen">
          <div className="pure-g">
            <div className="pure-u-1-1">
              <h1>⚠️</h1>
              <p>
                This browser has no connection to the Ethereum network. Please
                use the Chrome/FireFox extension MetaMask, or dedicated Ethereum
                browsers Mist or Parity.
              </p>
            </div>
          </div>
        </main>
      )
    );
  }

  renderNetworkMismatch() {
    return (
      this.props.networkMismatchComp || (
        <main className="container network-warning">
          <div className="pure-g">
            <div className="pure-u-1-1">
              <h1>⚠️</h1>
              <p>This dapp does not support this network.</p>
            </div>
          </div>
        </main>
      )
    );
  }

  renderNoAccounts() {
    return (
      <main className="container loading-screen">
        <div className="pure-g">
          <div className="pure-u-1-1">
            <h1>🦊</h1>
            <p>
              <strong>{"We can't find any Ethereum accounts!"}</strong> Please
              check and make sure Metamask or your browser are pointed at the
              correct network and your account is unlocked.
            </p>
          </div>
        </div>
      </main>
    );
  }
  render() {
    const { drizzleState, children, loadingComp } = this.props;
    if (drizzleState) {
      if (drizzleState.web3.status === "failed") {
        this.renderFailed();
      }

      if (drizzleState.web3.networkMismatch) {
        this.renderNetworkMismatch();
      }

      const noAccounts = Object.keys(drizzleState.accounts).length === 0;
      if (drizzleState.web3.status === "initialized" && noAccounts) {
        this.renderNoAccounts();
      }

      if (drizzleState.drizzleStatus.initialized) {
        return Children.only(children);
      }
    }

    return (
      loadingComp || (
        <main className="container loading-screen">
          <div className="pure-g">
            <div className="pure-u-1-1">
              <h1>⚙️</h1>
              <p>Loading dapp...</p>
            </div>
          </div>
        </main>
      )
    );
  }
}

LoadingContainer.propTypes = {
  drizzleState: PropTypes.object,
  children: PropTypes.node,
  loadingComp: PropTypes.node,
  errorComp: PropTypes.node,
  networkMismatchComp: PropTypes.node,
};

export default LoadingContainer;

[SeanJCasey]

Hey @adrianmcli , cool please do refactor as you see fit. I just copy/pasted/edited from the redux LoadingContainer for stylistic congruence (i.e., I implemented the networkMismatchComp the same way that the loadingComp and errorComp were already implemented), so if you refactor the new-context-api component, you might want to do so for the redux version as well.

[adrianmcli]

@SeanJCasey On second glance, it appears my testing of the UIs failed. It doesn't recognize and allow Ganache because Ganache doesn't have a stable network ID of 5557 as this line assumes.

Context:

@adrianmcli How about [1, 4] (Mainnet, Rinkeby) for the allowed networks? Then Ropsten etc will catch the network mismatch. Ganache is allowed by default, regardless of the values in the array.

[SeanJCasey]

@adrianmcli Hmm, interesting. I'll need to summon and defer to @cds-amal . We came to the conclusion that allowing port 5777 should (almost?) always be sufficient. Even if it isn't, we just need to make an addition to the readme that port 5777 is always allowed, but any custom ports need to be added (which might be obvious anyways if the user defines the networkMismatch config var).