First Step to #80: Let Children Render Display Data

[mwaeckerlin]

This is the first step to implement #80: You may add children to ContractData that render displayContent. For this, displayContent is added as displaycontentto the children's props.

This pull request implements #69 and obsoletes #70.

Usage

Simple Value

Usage example, first it renders SimpleValue=<value> directly in ContractData, because there are no children. Then on the next line, it passes the rendering to SimpleValue which is defined above and rebders the value in bold:

const SimpleValue = props => (
  <div>
    Simple Value: <b>{props.displaydata}</b>
  </div>
);

const Drizzle = props => (
  <LoadingContainer>
    <>
      <p>
        SimpleValue=<ContractData contract="Contract" method="getSimpleValue" />
      </p>
      <ContractData contract="Contract" method="getSimpleValue">
        <SimpleValue />
      </ContractData>
    </>
  </LoadingContainer>
);

Array

You can even use this already now to implement a custom rendering of arrays, e.g. Contract has a method getElements that returns an array of addresses:

const Addresses = props => (
    <ul>
      {props.displaydata.map(v => (
        <li key={v}>
          address: <strong>{v}</strong>
        </li>
      ))}
    </ul>
);

const Drizzle = props => (
  <LoadingContainer>
      <ContractData contract="Contract" method="getElements">
        <h1>List of Addresses</h1>
        <Addresses />
      </ContractData>
  </LoadingContainer>
);

Change Log

implemented first step: allow nested child components to Contract data, and if they exist, pass the rendering to the child components; it is passed as displaydata in lower case only to prevent a warning

[honestbonsai]

Thanks a lot for your hard work and detailed issue and PR @mwaeckerlin!

Not sure if this is a work in progress or not, but there's still a few things that I'd like to see before accepting the PR:

1. Add an implementation for the new context components as well.
2. Implement this in both the test apps so people know how to use it.
3. Update the docs to reflect this new functionality.

FYI: https://github.com/trufflesuite/drizzle-react-components/blob/develop/CONTRIBUTING.md#tips-for-getting-your-pr-merged

[mwaeckerlin]

Thank you for your comments, I'll check them and update my changes.

[mwaeckerlin]

@honestbonsai, what I love in the drizzle-react-component is, how simple and easy you get your results. Now I studied render props, and IMHO, the idea of using child components is the simpler solution from the library user's perspective.

Please note, that normally a <ContractData> does not have children, so you loose nothing if you don't need the feature. But if you have children, then you want the prop in all children, so yes, it should propagate it to all children.

If you agree, I'd like to stay with this solution and I'll provide you with the other stuff you requested (chanes in tests and documentation). Agree?

[mwaeckerlin]

I cannot run the test-app/app: TypeError: Cannot read property 'AccountData' of undefined → @honestbonsai

[honestbonsai]

@mwaeckerlin For the AccountData issue, wipe your node_modules and reinstall. DRC 1.3 was released quite recently.

[honestbonsai]

@mwaeckerlin

what I love in the drizzle-react-component is, how simple and easy you get your results. Now I studied render props, and IMHO, the idea of using child components is the simpler solution from the library user's perspective.

Render props are a pretty established React pattern these days, so I don't anticipate many users being too confused by it. However passing all props down to all children when the children might not need it, is a less common pattern (and I'd argue, less precise as well). I prefer being explicit over being implicit, providing data only to the consumers that need it, and sticking to established patterns so the user is aware what's going on.

Yes, you are right in saying this introduces some more overhead for the user, but I'd argue that the overhead incurred is far outweighed by the flexibility introduced by the render props pattern.

Please note, that normally a does not have children, so you loose nothing if you don't need the feature. But if you have children, then you want the prop in all children, so yes, it should propagate it to all children.

I'm not sure we can generalize so far to say that all users want their children to receive these props. Rather than needing to guess at what our users want to do with the data, would it not just be easier to provide users the tools to perform this task how they see fit?

Perhaps the user doesn't want the prop to be called displayData? Perhaps it overrides one of the user's custom props that has the same name? We have no idea of how they want their downstream consumers to consume the data currently. So I argue that it's easier to just give the users the tools they need to adapt to their own situation.

[mwaeckerlin]

Ok, I'll check, how it looks like with render props.

@mwaeckerlin For the AccountData issue, wipe your node_modules and reinstall. 1.3 was released quite recently.

Nope, that does not help. I forked on Friday, so my installation it was at most 4 days old.

Shouldn't package.json in test-app/appreference excatly the versions that are required?

[mwaeckerlin]

@honestbonsai, your approach with render props means, that I have to split all your classes in two new classes, one that only gets the data and calls the render prop and the other that implements the default rendering.

Is this what you want?

If yes, how would you like to have the naming for these two classes?

Well, the default render class should have the existing name, e.g. ContractData, otherwise we would break all existing code. But what name would you like for the new class that sets up the displayData and calls the render prop?

[honestbonsai]

@mwaeckerlin

your approach with render props means, that I have to split all your classes in two new classes, one that only gets the data and calls the render prop and the other that implements the default rendering.

Not sure why we would be needing to split the class into 2? My initial assumption would be that the class (for example ContractData) API would look something like this.

// Approach 1
<ContractData contract="Contract" method="getSimpleValue">
      {({ data }) =>  <SimpleValue {data} />} /
</ContractData>

or

// Approach 2
<ContractData 
  contract="Contract" 
  method="getSimpleValue" 
  render={({ data }) =>  <SimpleValue {data} />} 
/>

In both cases, wouldn't it just be easier to render the current defaults if the render prop is not provided? Still not quite clear why we need a new class for this. I feel like it can be just done inside the current one.

[mwaeckerlin]

Still learning render props. I'll try your idea, please wait …

[honestbonsai]

Also please refer to https://github.com/trufflesuite/drizzle-react-components/blob/develop/CONTRIBUTING.md#getting-started

The npm version of DRC shouldn't stop you from developing locally.

[mwaeckerlin]

I apologize, @honestbonsai, you are smarter than me — at least regarding react. Your solution is better than mine, I'll change my proposal to:

const version = displayData => (
  <div>
    Version: <b>{displayData}</b>
  </div>
);

const addresses = displayData => (
  <>
    <h1>List of Contracts</h1>
    <ul>
      {displayData.map(v => (
        <li key={v}>
          address: <strong>{v}</strong>
        </li>
      ))}
    </ul>
  </>
);

const Drizzle = props => {
  console.log("Drizzle.props", props);
  return (
    <LoadingContainer>
      <>
        <ContractData contract="Contract" method="getSimpleValue" render={simpleValue} />
        <ContractData
          contract="Contract"
          method="getElements"
          render={addresses}
        />
      </>
    </LoadingContainer>
  );
};

I will update my push request as soon as I have added documentation and an example.

[mwaeckerlin]

@honestbonsai, all updated according to your requests. I am sorry for some minor reformatting, this time I used Visual Coder Studio instead of Emacs, and that does automatic reformatting.

I tested test-app-legacy-context and could not test test-app.

Please note that of course you want to refer to the local drizzle-react-components in the test apps, since they fit to your local code, so I changed that in their package.json.

If you need anything else, please let me know,

[mwaeckerlin]

Now with the correct dependencies to the local drizzle-react-components, even the new test-app works as expected.

[mwaeckerlin]

@honestbonsai, do you have everything to restart the review and to accept the changes?

[mwaeckerlin]

Everything changed according to your inputs, @honestbonsai, thank you.

Even though I changed it back, I'd still strongly recommend to refer to the local drizzle-react-components library from the examples. Simply because only this works, version ^1.3.0 does not support the new… stuff and it does not render arrays. And my extension will not work either. Only with the local reference, the example code matches the library implementation. Version 1.3.0 is buggy and lacking features.

[honestbonsai]

@mwaeckerlin Thanks for bringing the issues that 1.3.0 has to our attention, it's important, but I think it belongs in its own issue and PR. Just trying to keep this PR tightly scoped.

If you can develop your features locally and I can test it, it's good enough for me for this PR.

I'll open up a new issue to investigate it. I'm pretty sure I made an attempt at fixing it already in #84, but I guess it didn't work.

[honestbonsai]

@mwaeckerlin Looks good to me! Thanks for all the work you put into this PR!

[mwaeckerlin]

多谢．Thank you for accepting my pull request.

My next steps:

• I'll add the same flexibility to the ContractForm.
• I'll check what's the best way to support dynamic contracts (Contracs instanciated from addresses in other contracts)
• !'ll add something for getting a contract's balance.

Now that I know what you prefer, I hope next time it will be easier.