Move to Node-style callbacks in every subscription #229
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is needed to pass errors with subscriptions, the same way we do it for the async functions by throwing errors. Contains the following changes: - Add the add SubscriptionCallback type. - Add types to all the subscriptions methods. - Move all the subscription methods to use node-style callbacks. - Update documentation and examples.
Codecov Report
@@ Coverage Diff @@
## master #229 +/- ##
==========================================
- Coverage 34.93% 34.43% -0.50%
==========================================
Files 98 99 +1
Lines 1792 1838 +46
Branches 274 295 +21
==========================================
+ Hits 626 633 +7
- Misses 1166 1205 +39
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Merged
0xGabi
approved these changes
Aug 24, 2020
There was a problem hiding this comment.
Looking good!
I'm not super convinced by the order of the parameters. Now that I have a better understanding of the pattern Error-first callbacks I agree with the decisions.
|
Merge this one to make #233 easier to review. |
bpierre
commented
Aug 24, 2020
| const apps = await parseApps(result, organization) | ||
| return apps[0] | ||
| } | ||
| (result) => parseApp(result, organization) |
There was a problem hiding this comment.
@0xGabi Wondering if you tested this? Because the query is ORGANIZATION_APPS with a limit of 1 since we added the filters (the single app query was only working by address).
There was a problem hiding this comment.
Apology, I didn't check on my local the change as it was small and seems straight forward but I got confused with how we were handling APP_BY_ADDRESS query.
bpierre
added a commit
that referenced
this pull request
Aug 26, 2020
This commit contains various improvements for the React API. ## Loading behavior Since we replaced the GraphQL subscriptions by queries with polling (see #203), the hooks provided by the React API were updating too often, and incorrectly (see #216). This version is now taking care of only triggering updates when a changed happened, and to only use loading states when necessary. ## createAppHook(): allow connector configuration to be set App connectors can receive a source connector and its options, but this wasn’t possible when using `createAppHook()`. A second parameter has been added, to either pass a specific connector or to change its options: import connectVoting from '@aragon/connect-voting' import { createAppHook } from '@aragon/connect-react' const useVoting = createAppHook(connectVoting, ['thegraph', { subgraphUrl: 'https://…' }]) ## createAppHook(): dependencies array Hooks created through `createAppHook()` now accept a dependency array, that can get used to refresh its callback. When no dependency array is passed, the callback never updates. import connect from '@aragon/connect' import connectVoting from '@aragon/connect-voting' import { createAppHook, useApp } from '@aragon/connect-react' const useVoting = createAppHook(connectVoting) function App() { const [page, setPage] = useState(0) const [voting] = useApp('voting') const [votes] = useVoting(voting, app => ( app.votes({ first: 10, skip: 10 * page }) ), [page]) return ( <div> <button onClick={() => setPage(page + 1)}>prev</button> <button onClick={() => setPage(page - 1)}>next</button> </div> ) } ## Populating status.error rather than throwing errors In many cases, errors were thrown rather than populating the `status.error` object in the React API. It is an issue that was also happening with the JS API (see #229). It has now been fixed. import connect from '@aragon/connect' import connectVoting from '@aragon/connect-voting' import { createAppHook, useApp } from '@aragon/connect-react' const useVoting = createAppHook(connectVoting) function App() { const [voting, votingStatus] = useApp('voting') const [votes, votesStatus] = useVoting(voting, app => app.votes()) if (votingStatus.error) { return ( <h1>Error: {votingStatus.error.message}</h1> ) } if (votesStatus.error) { return ( <h1>Error: {votesStatus.error.message}</h1> ) } return ( // … ) } ## App methods subscriptions Prior to this version, it was not possible to subscribe to app methods using `createAppHook()`. The fetched data was returned once, and only updated when the component was mounted again. The feature has been added, and we can now subscribe to any app methods, alongside the core ones (`useApp()`, `usePermissions()`, …). Every part of the React API is now listening to changes and reacting accordingly. To use this feature, the subscription methods (`onX`) should be called, but without passing a callback. `createAppHook()` will take care of handling the subscription. import connect from '@aragon/connect' import connectVoting from '@aragon/connect-voting' import { createAppHook, useApp } from '@aragon/connect-react' const useVoting = createAppHook(connectVoting) function App() { const [voting, votingStatus] = useApp('voting') // To enable a subscription, use app.onVotes() rather than app.votes(). // Note that the default value can be set using the assignment default. const [votes = [], votesStatus] = useVoting(voting, app => app.onVotes()) return ( // … ) } Of course, it also works with the dependencies array, taking care of unsubscribing and subscribing to the new query: import connect from '@aragon/connect' import connectVoting from '@aragon/connect-voting' import { createAppHook, useApp } from '@aragon/connect-react' const useVoting = createAppHook(connectVoting) function App() { const [page, setPage] = useState(0) const [voting] = useApp('voting') const [votes] = useVoting(voting, app => ( // Note the app.onVotes() rather than app.votes(). app.onVotes({ first: 10, skip: 10 * page }) // The subscription will refresh every time `page` changes. ), [page]) return ( // … ) } Important: this only works with app connectors enabling this syntax. All the core app connectors have been updated for it, but third party app connectors will require to be updated too.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a breaking change for app connectors and consumers (with the exception of
@aragon/connect-react). It is necessary to pass errors with subscriptions, the same way we do it for the async functions by throwing errors.A compatible solution could be to pass the error as a second parameter, but many people are used to Node-style callbacks, so I think it would be too weird to invert the parameters.
Contains the following changes:
SubscriptionCallbacktype.