Conversation
Codecov Report
@@ Coverage Diff @@
## master #52 +/- ##
=======================================
Coverage 25.25% 25.25%
=======================================
Files 57 57
Lines 986 986
Branches 163 163
=======================================
Hits 249 249
Misses 737 737
Continue to review full report at Codecov.
|
What is the strategy for connecting to a particular app in a react context? Will that be the domain of the app connector to define? |
I am currently adding a import { connect } from '@aragon/connect'
import VotingConnector from '@aragon/connect-thegraph-voting'
const org = await connect('myorg.aragonid.eth', 'thegraph')
let votesSubscription
org.onApps(apps => {
const voting = apps.find(app => app.appName.startsWith('voting.'))
const votingConnector = await voting.connect(VotingConnector)
votesSubscription?.unsubscribe()
votesSubscription = votingConnector.onVotes(null, votes => {
console.log(votes)
})
})Then I was thinking of adding the equivalent in React: import { useApps, useAppSubscription } from '@aragon/connect-react'
import VotingConnector from '@aragon/connect-thegraph-voting'
function App() {
const [apps] = useApps()
const voting = apps.find(app => app.appName.startsWith('voting.'))
// We would take care of reusing the connection internally, for any given app + app connector.
// Params are: app, app connector, method, method params.
const votes = useAppSubscription(voting, VotingConnector, 'onVotes', {}) ?? []
return (
<ul>
{votes.map(vote => (
<li>{vote.id}</li>
))}
</ul>
)
}
export default () => (
<Connect location="myorg.aragonid.eth" connector="thegraph">
<App />
</Connect>
)Another option would be to reuse the app filters, to let people declare them at the import { useApps, useAppSubscription } from '@aragon/connect-react'
import VotingConnector from '@aragon/connect-thegraph-voting'
function App() {
const [apps] = useApps()
const voting = apps.find(app => app.appName.startsWith('voting.'))
// No need to pass the connector here, it’s already set by Connect.
const votes = useAppSubscription(voting, 'onVotes', {}) ?? []
// It could also be a good idea to extend App with a connector property.
// We might also want to consider having a similar system on the core library,
// so that users could pass all their app connectors at the connect() level.
return (
<ul>
{votes.map(vote => (
<li>{vote.id}</li>
))}
</ul>
)
}
export default () => (
<Connect
location="myorg.aragonid.eth"
connector="thegraph"
appConnectors={[
// Everything app that matches would use the passed connector.
// These rules would be applied one after another, so that we could
// for example match all the voting apps with one connector, except one.
[{ appName: 'voting.aragonpm.eth' }, VotingConnector],
[{ appName: 'token-manager.aragonpm.eth' }, TokenConnector],
// Like the .app() and .apps() filters
[{ address: '0xcafe…' }, MyConnector],
// It would also accept a function
[app => app.appName = 'something.aragonpm.eth', MyOtherConnector],
]}
>
<App />
</Connect>
) |
Evalir
left a comment
There was a problem hiding this comment.
Asking some questions! In general it's looking good to me. :)
0xGabi
left a comment
There was a problem hiding this comment.
Solid PR, I left a few questions that we can discuss, and if they make sense include in a new PR.
Regardless LGTM! 🙌
|
We spoke about this offline so just to codify my thoughts on the error and loading status handling. I think having two items in the initial return might be a bit nicer:
And accessing properties:
Or perhaps:
I don't know if you had any further thoughts @bpierre |
| type LoadingStatus = { | ||
| error: Error | null | ||
| loading: boolean | ||
| retry: () => void |
There was a problem hiding this comment.
I think this is fine for this version, but since we are dealing with subscriptions, we should probably auto-retry for the next one.
errorwould still get populated (and emptied on success).- It would still be possible to call
retry, e.g. we could imagine a user wanting to retry immediately by clicking a button. - We could have a
retryEveryprop on the provider, that either accept a number (milliseconds), or a function implementing a delay based on the number of attempts. Setting it to-1would disable the automatic retry behavior.
There was a problem hiding this comment.
Since we are dealing with subscriptions
Outside of the retryEvery prop or a user calling retry, the subscription would only help here if the underlying data was updated in the mean time, right? Or could you see a case where you hadn't connected / had some sort of corruption, and the GraphQL subscription just magically fixed it at a later point?
There was a problem hiding this comment.
This will really be useful e.g. for the apps, but it should be possible to wait for an organization to arrive, for example if it just got created and we are waiting for it to appear on the Subgraph. Otherwise, we could also imagine that the first attempt fails because of a connection issue, and we keep retrying until it works.
| } | ||
|
|
||
| return ( | ||
| <Group name="Organization"> |
There was a problem hiding this comment.
I think this is implicit as they are used in the OrgGroups component. The one that makes me wonder is OrgPermissions because we are using loading in that one. Is this way of thinking correct?
| export default function OrgPermissions() { | ||
| const [permissions, { loading }] = usePermissions() | ||
| return ( | ||
| <Group name="Permissions" loading={loading}> |
There was a problem hiding this comment.
Related to the above comments. Do we need loading here?
There was a problem hiding this comment.
Yes: if we are waiting for the first onPermissions() call, or if the org itself is loading.
| type ConnectProps = { | ||
| children: React.ReactNode | ||
| connector: ConnectorDeclaration | ||
| location: string |
There was a problem hiding this comment.
I quite like location I think in the context of the Connect component should not be that vague. Otherwise how you feel about identity?
Builds on #111
Summary
This PR adds an initial version of the React library.
<Connect />
A decision was made to move the configuration on the
<Connect />component, which makes it possible to declare it once in the app. The idea is to optimize for the use case of connecting to a single organization, and interacting with it. Users can still declare<Connect />in different places to connect to more than one organization, or use@aragon/connectdirectly.Hooks for subscriptions only
The only hooks provided are abstracting the subscriptions. Async calls are left as promises for now. This is something we might want to reconsider in the future depending on usage.
@aragon/connect re-exports
The entire @aragon/connect is being exported by the React library. This is to make it easier when adding new types, but we might prefer to list the exports instead.
App connectors
There is no hook allowing to subscribe to apps through their connectors yet. The plan is to do it as a second step, once they can be instantiated from the connection in the core library.
Organization
The
Organizationobject has been left as it is, including methods that are not needed with the react library like.apps(). This is to avoid having different types for the React library and for the others.# Using Aragon Connect with React
Introduction
Aragon Connect provides a series of utilities that makes it easier to use in React environments.
It consists of the
<Connect />component, on which a connection to an organization is described, and a series of hooks:useApp(),useApps(),useOrganization(),usePermissions().Usage
API
<Connect />
This component is required in order to use the provided hooks.
locationStringconnectorConnector | [String, Object] | StringConnectorinstance, and either a string or a tuple for embedded connectors and their config.optionsObjectoptions.readProviderEthereumProvideroptions.chainIdNumber1.useOrganization()
[Organization | null, { loading: boolean, error: null | Error, retry: Function }]useApp(appFilters)
appFilterStringor object (optional)0x, and byappNameotherwise. SeeappFilter.addressandappFilter.appNameto set them explicitly. For the time being, only one type of filter can get passed at a time.appFilter.addressStringappFilter, but makes the selection byaddressexplicit.appFilter.appNameStringappFilter, but makes the selection byappNameexplicit.[App | null, { loading: boolean, error: null | Error, retry: Function }]useApps(appFilters)
appFilterStringorString[]or object (optional)0x, and byappNameotherwise. When an array is passed, the first entry determines the type of filter. SeeappFilter.addressandappFilter.appNameto set them explicitly. For the time being, only one type of filter can get passed at a time.appFilter.addressStringorString[]appFilter, but makes the selection byaddressexplicit.appFilter.appNameStringorString[]appFilter, but makes the selection byappNameexplicit.[App[], { loading: boolean, error: null | Error, retry: Function }]usePermissions()
[Permission[], { loading: boolean, error: null | Error, retry: Function }]