Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

Skeleton Single Page Application #18

Merged
merged 14 commits into from
Aug 2, 2019
Merged

Skeleton Single Page Application #18

merged 14 commits into from
Aug 2, 2019

Conversation

taggartbg
Copy link
Contributor

@taggartbg taggartbg commented Jul 30, 2019

This is a very basic SPA skeleton to get started. It mimics the Random Beacon dApps (staker dashboard + emitter/requester) in packages and structure.

Includes:

  • Very basic component tree and Routing
  • Some basic styles to get started
  • Entry points for reducers and sagas
  • Basic context wrappers and HOCs for web3 and an example of a contract

TODO:

Closes #5

Implementer: @taggartbg
Feature buddy: @liamzebedee

@taggartbg taggartbg requested review from liamzebedee and pdyraga July 30, 2019 13:55
@liamzebedee
Copy link
Contributor

liamzebedee commented Jul 30, 2019

To recap: from the tBTC planning today, we summarised the desired qualities as something like:

  1. ease of use for a newcomer (think hackathon)
  2. speed of setup (think create-react-dapp)
  3. robust engineering (React, state mgmt)
  4. modern defaults (CSS, ES6)

With an obvious tradeoff between 1/2 and 3/4. 😆 let the games begin! 🔥

@liamzebedee
Copy link
Contributor

  • component routing is much better. I didn't go with it because I couldn't get it setup with Next.js and server-side rendering. But I think it's the way to go.
  • LESS CSS is nice, I think it's useful primarily for naming colours as variables. Inheritance, not so much.
    • styled-components might be too sugarery for some still, although I love it (says a diabetic) - no syntax highlighting for the template strings, and putting CSS in files is nice and familiar and we don't want to break away from that
    • that being said, we definitely need some form of scoped CSS. It's a mess to code without it.
  • no hot module reload yet - this is pretty essential for iterative development, found it extremely useful with Next.js.
  • checking for whether we have unlocked Metamask - definitely missing in Setup dApp skeleton #7
  • web manifest - missing in Setup dApp skeleton #7, is it useful?

Some questions for @taggartbg :

  • the global contract context, given to each contract. I like the idea, could you elaborate? My hunch is that async flow should be done in sagas 80% of the time, but it is very useful for tracking transaction status without overhead.
  • I see you've got withWeb3, withAccount and some other HOC's. Could you give an example? They look super useful

Early reviews :)

@taggartbg
Copy link
Contributor Author

taggartbg commented Jul 30, 2019

Quick answers (running to an appointment, will update further + review #7 afterwards)

CSS scoping: Yeah, definitely the major downfall of a stylesheet vs inline, but I think component-level class names is usually enough to tackle the majority of naming collisions

Hot reloading: Very good call. I haven't done hot reloading in 2019, but it looks like the tooling has gotten stronger (react-app-rewired and react-hot-loader seem like good tools)

Web Manifest: Create-react-app makes it and I guess its good to have for browsers that want to use it (usually mobile)

Global contract context: Absolutely true, now that I'm thinking about it, it is probably useless, as we'll definitely want sagas controlling that flow. In theory we could connect the action literals to the contract context wrapper and pass them down that way, but I don't see that being stronger than using module imports + connecting where needed.

Web3 Context: I threw an example in App.js grabbing the balance from Context. I imagine it'd be useful for accessing the connected web3 instance, getting the balance to do client-side checks of funds, reacting to changes in account / network, grabbing a list of ERC20 tokens in MetaMask, etc.

@Shadowfiend Shadowfiend mentioned this pull request Jul 30, 2019
This was referenced Jul 31, 2019
@pdyraga
Copy link
Member

pdyraga commented Jul 31, 2019

Let's undraft this PR and work on merging it today. All the items from TODO list should be implemented in subsequent PRs.

@taggartbg Before we do anything else, you need to set up commit signing and rebase this branch to sign all the commits.

@pdyraga
Copy link
Member

pdyraga commented Jul 31, 2019

Assigned @liamzebedee as feature buddy.

@Shadowfiend
Copy link
Contributor

Commits are signed, just attributed to an email that needs to be added to @taggartbg’s GitHub profile.

@taggartbg taggartbg marked this pull request as ready for review July 31, 2019 21:20
@taggartbg
Copy link
Contributor Author

I used the saga + reducer workflow from PR #7 and adapted for it to here as @liamzebedee had already completed that work. This should be ready to be merged + built on.

Copy link
Contributor

@liamzebedee liamzebedee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't seem to work on Metamask when on Ropsten testnet?

image

Homepage layout is wrong

image

<Web3Wrapper>
<Route path="/" exact component={Home} />
<App>
<Route path="/start" component={Start} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the names - pay, confirm, prove -- really simple to get


export function submitProof({ history }) {
return {
history,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not pass around the history object in actions. I'm sure there's a redux helper pkg here somewhere? (since React Router is ubiquitous)

🙏

Copy link
Contributor Author

@taggartbg taggartbg Aug 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I definitely hear you.

A couple immediate thoughts:

  • pass the history to a custom middleware that injects it into sagas when initializing (also pass to <Router>)
  • Create a singleton module and import both when creating routes and whenever a saga is called

I'll also google around, let me know if either of those sound better or worse to you!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Middleware is an interesting idea! I think it's probably best to stick away from the Not Invented Here -style solutions, because routing+redux is such a common combo, it's bound to have been solved.

How does https://github.com/supasate/connected-react-router look to you?

class Web3Wrapper extends Component {
state = {}

componentDidMount() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice pattern, TIL!

package.json Outdated
"subPackages": [
"client",
"lib/tbtc-helpers"
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing ,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unresolving because it wasn't removed and it's the reason why npm install is failing on my machine:

➜  tbtc-dapp git:(skeleton) ✗ npm install
npm ERR! file /Users/piotr/go/src/github.com/keep-network/tbtc-dapp/package.json
npm ERR! code EJSONPARSE
npm ERR! JSON.parse Failed to parse json
npm ERR! JSON.parse Unexpected token } in JSON at position 1365 while parsing near '.../tbtc-helpers"
npm ERR! JSON.parse   ],
npm ERR! JSON.parse }
npm ERR! JSON.parse '
npm ERR! JSON.parse Failed to parse package.json data.
npm ERR! JSON.parse package.json must be actual JSON, not just JavaScript.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/piotr/.npm/_logs/2019-08-02T08_46_19_368Z-debug.log

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taggartbg did you resolve this perhaps?

(it's possible this was an accidental button mash from me, due to my stupid Mac keyboard)

@taggartbg
Copy link
Contributor Author

RE: Ropsten - I meant to subscribe to network changes, oops! Will add now

}

getAccountInfo = async () => {
console.log("CHANGE!")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was it intentionally committed?

@pdyraga
Copy link
Member

pdyraga commented Aug 2, 2019

Does not seem to be working:

image

^ I was already logged in to metamask when opening the page. If I log out, open the app and log in again I am running into the same problem.

Also, does not seem to work when I switch the network:

image

Not sure if this is related but the web3 version returned by browsers is 0.2x.x while the package.json says I should be at ^1.2.0

web3.version
{api: "0.20.7", getNode: ƒ, getNetwork: ƒ, …}

package.json Outdated
"subPackages": [
"client",
"lib/tbtc-helpers"
],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unresolving because it wasn't removed and it's the reason why npm install is failing on my machine:

➜  tbtc-dapp git:(skeleton) ✗ npm install
npm ERR! file /Users/piotr/go/src/github.com/keep-network/tbtc-dapp/package.json
npm ERR! code EJSONPARSE
npm ERR! JSON.parse Failed to parse json
npm ERR! JSON.parse Unexpected token } in JSON at position 1365 while parsing near '.../tbtc-helpers"
npm ERR! JSON.parse   ],
npm ERR! JSON.parse }
npm ERR! JSON.parse '
npm ERR! JSON.parse Failed to parse package.json data.
npm ERR! JSON.parse package.json must be actual JSON, not just JavaScript.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/piotr/.npm/_logs/2019-08-02T08_46_19_368Z-debug.log

@Shadowfiend
Copy link
Contributor

Add localstorage support?

Probably a good idea to move this to a separate/future issue, as it won't be necessary for the demo, right?

@mhluongo
Copy link
Member

mhluongo commented Aug 2, 2019

Created an issue for localStorage.

@liamzebedee
Copy link
Contributor

@pdyraga seems to be working for me when loading when already logged in, tested on Ropsten/Kovan. Ran web3.version in console and got:

web3.version
{api: "0.20.7", getNode: ƒ, getNetwork: ƒ, …}

However when Metamask is not logged in, and then I log in, it does not respond to the change.

- Remove trailing comma
- Add sleep to avoid race condition between CRA and less compiler
@taggartbg
Copy link
Contributor Author

  • Removed trailing comma from package.json (rebase snafoo)
  • Added a sleep to npm's start-js task, I was running into a race condition between CRA and the less compiler: Development page blank on first load facebook/create-react-app#5809
  • Tried switching networks and logging in - its working fine for me. Make sure you have the latest MetaMask @pdyraga , if you run window.ethereum you should get an object back

@pdyraga
Copy link
Member

pdyraga commented Aug 2, 2019

I have 6.7.3 Metamask version.

I was logged in when opened the application and saw the screen below. Then tried to log out and log in again and I had the same problem. Switching network does not help.
image

Copy link
Contributor

@liamzebedee liamzebedee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code seems fine, history works. But the page still doesn't refresh automatically after you unlock Metamask.

From what I can gather from MakerDAO's dapp, the only way you can detect the change is to poll the window.xyz. Also found an answer here which should prove useful https://ethereum.stackexchange.com/questions/43572/how-to-auto-reload-dapp-when-metamask-is-unlocked-or-locked-by-user

Copy link
Member

@pdyraga pdyraga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not want to block merging. If it works for you both we can go ahead and debug the problem later. Just need to make sure at least one other person checks it later.

@pdyraga
Copy link
Member

pdyraga commented Aug 2, 2019

But the page still doesn't refresh automatically after you unlock Metamask.

If it is some more time-consuming work, we may want to do it in a separate PR fwiw.

<Route path="/" exact component={Home} />
<App>
<Route path="/start" component={Start} />
<Route path="/pay" exact component={Pay} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like it could be “deposit”?

@pdyraga pdyraga merged commit d55f59b into master Aug 2, 2019
@pdyraga pdyraga deleted the skeleton branch August 2, 2019 19:05
@taggartbg
Copy link
Contributor Author

@liamzebedee If you have the window.ethereum object, you SHOULD be able to subscribe to account / network change events. On v6.7.3 it works for me, not sure why its not working for you two.

We could make a new Issue, but it seems like two issue:

  • one that its not detecting @pdyraga 's web3 provider at all
  • Its not responding to change events or MM isn't firing them

Seems worth fixing ASAP

@Shadowfiend
Copy link
Contributor

I think it probably makes sense to circle back to the Metamask issues next week. Basic reasoning: if we can't get Metamask working, we can mock out the calls to it and have a good demo. If, on the other hand, we fall short on the app flow/design, having Metamask working doesn't get us much from the demo's perspective.

@pdyraga
Copy link
Member

pdyraga commented Aug 2, 2019

I think it probably makes sense to circle back to the Metamask issues next week. Basic reasoning: if we can't get Metamask working, we can mock out the calls to it and have a good demo. If, on the other hand, we fall short on the app flow/design, having Metamask working doesn't get us much from the demo's perspective.

Yeah, I was thinking about contingency plan today and absolutely agree. We can also show the process from UI with mocks and then "hey, see how it works under the hood" and show (even a video) of how we do it with 4 consoles and clients using demo scripts we already have working. Worth discussing elsewhere but there is an alternative so we are safe.

@liamzebedee liamzebedee restored the skeleton branch August 5, 2019 09:18
@taggartbg taggartbg deleted the skeleton branch November 13, 2019 20:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DApp Skeleton
5 participants