Replace GraphQL subscriptions by queries + polling #203

bpierre · 2020-08-14T00:32:09Z

The Graph is having issues with GraphQL subscriptions, so we are moving to queries with polling for the time being. Once subscriptions get enabled again on The Graph, we will move back to GraphQL subscriptions.

Also in this PR:

nodejs/ demos update.
Add the possibility to consume the organization connector from any app connector, allowing to retrieve its options for example. This was done so that app connectors using TheGraph can inherit from pollInterval defined in the main connector.

The Graph is having issues with GraphQL subscriptions, so we are moving to queries with polling for the time being. Once subscriptions get enabled again on The Graph, we will move back to GraphQL subscriptions.

codecov · 2020-08-14T00:35:08Z

Codecov Report

Merging #203 into master will increase coverage by 0.91%.
The diff coverage is 34.04%.

@@            Coverage Diff             @@
##           master     #203      +/-   ##
==========================================
+ Coverage   34.96%   35.88%   +0.91%     
==========================================
  Files          98       99       +1     
  Lines        1799     1856      +57     
  Branches      276      304      +28     
==========================================
+ Hits          629      666      +37     
- Misses       1170     1190      +20

Flag	Coverage Δ
#unittests	`35.88% <34.04%> (+0.91%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/connect-agreement/src/connect.ts	`28.57% <0.00%> (-7.80%)`	⬇️
...ages/connect-core/src/connections/ConnectorJson.ts	`0.00% <0.00%> (ø)`
packages/connect-core/src/utils/app-connectors.ts	`0.00% <ø> (ø)`
packages/connect-thegraph/src/connector.ts	`0.00% <0.00%> (ø)`
...ckages/connect-thegraph/src/core/GraphQLWrapper.ts	`0.00% <0.00%> (ø)`
packages/connect-voting/src/connect.ts	`28.57% <0.00%> (-11.43%)`	⬇️
packages/connect-voting-disputable/src/connect.ts	`28.57% <10.00%> (-11.43%)`	⬇️
packages/connect-finance/src/connect.ts	`28.57% <11.11%> (-11.43%)`	⬇️
...onnect-voting-disputable/src/thegraph/connector.ts	`51.89% <53.33%> (ø)`
packages/connect-tokens/src/thegraph/connector.ts	`75.00% <80.00%> (-1.48%)`	⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12dec7e...34ad85e. Read the comment docs.

Evalir

LGTM! Such a simple change. :)

On a separate note: I'm guessing there are other examples out of date, right?

packages/connect-agreement/src/thegraph/queries/index.ts

packages/connect-thegraph/src/core/GraphQLWrapper.ts

This is so app connectors don’t have to change it to `query`, then back to ${type} whenever we will support GraphQL subscriptions again.

packages/connect-thegraph/src/core/GraphQLWrapper.ts

0xGabi

Changes look great, I left a comment on how we may want to generalize the GraphQLWrapper POLL_INTERVAL as a constructor argument so app connectors and even users doing custom queries may get more granularity on the interval, as discussed.

Two for now: - Parcel - Webpack with ts-loader

Also remove the unecessary passing of The Graph URLs.

bpierre · 2020-08-14T16:10:17Z

On a separate note: I'm guessing there are other examples out of date, right?

Indeed! All the examples should be up to date now: #205

pollInterval is an option that can be set on all the TheGraph connectors. This commit also adds the possibility to consume the organization connector from any app connector, allowing to retrieve its options for example. This was done so that app connectors using TheGraph can inherit from pollInterval defined in the main connector.

0xGabi

Great work, I leave a few considerations for a few of the types. I also notice that the Tokens app connector is a bit different of the rest. I wonder if after merging this one we might want to refactor it to keep a single way of implementing connectors.

Other than that everything looks good 🙌

0xGabi · 2020-08-15T23:57:47Z

packages/connect-agreement/src/thegraph/connector.ts

@@ -28,16 +28,25 @@ export function subgraphUrlFromChainId(chainId: number) {
  return null
 }

+type AgreementConnectorTheGraphConfig = {


Maybe we can generalize this type for every app connector? Then if the specific app connector wants can extent it to include extra configurations.

Do you mean for our connectors only, or as a type that would be exported by connect-core for any connector to extend the base config? The way I see it, a connector configuration is the responsibility of the connector defining it, even if they happen to be identical in our case because we follow the same conventions. What do you think?

packages/connect-core/src/connections/IOrganizationConnector.ts

0xGabi · 2020-08-16T00:34:34Z

packages/connect-finance/src/connect.ts

@@ -1,27 +1,33 @@
-import { createAppConnector, Network } from '@aragon/connect-core'
-import { IFinanceConnector } from './types'
+import { createAppConnector } from '@aragon/connect-core'
 import Finance from './models/Finance'
 import FinanceConnectorTheGraph, {
  subgraphUrlFromChainId,
 } from './thegraph/connector'

 type Config = {


I notice every app connector also defines this type. I wonder if it's ok to be redundant on those types or we should apply the DRY principle.

I guess in our case we might want to have a base that we would extend, but I also like the idea of keeping them separate, even if it require some repetition, because it makes it easy for connector authors to take one and use it as a bootstrap. What do you think?

packages/connect-thegraph/src/core/GraphQLWrapper.ts

packages/connect-tokens/src/connect.ts

0xGabi · 2020-08-16T00:56:32Z

packages/connect-tokens/src/thegraph/connector.ts


    const token = await gql.performQueryWithParser<Token>(
      queries.TOKEN('query'),
-      { tokenManagerAddress: appAddress },
+      { tokenManagerAddress: config.appAddress },


Looking through the other connectors we are handling the Tokens app connector a bit differently, using the create method. I wonder if we might want to refactor it a bit to keep a single source of truth on how connectors are implemented?

Using create() is needed in the tokens app connector, because it’s the only one that need to instantiate asynchronously. What would you think of moving all the connectors to an async create(), even if they don’t need it (yet)?

packages/connect-voting-disputable/src/__test__/models/votes.test.ts

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.

bpierre added 2 commits August 14, 2020 01:14

Fix nodejs/ examples

d50125f

Replace GraphQL subscriptions by queries + polling

765c320

The Graph is having issues with GraphQL subscriptions, so we are moving to queries with polling for the time being. Once subscriptions get enabled again on The Graph, we will move back to GraphQL subscriptions.

bpierre requested review from 0xGabi, Evalir and rperez89 August 14, 2020 00:32

bpierre added 3 commits August 14, 2020 01:35

nodejs demo (subscriptions-voting): tweak display

c867c2c

Remove comments

a3243db

Fix request policy

4578a10

Evalir approved these changes Aug 14, 2020

View reviewed changes

packages/connect-agreement/src/thegraph/queries/index.ts Outdated Show resolved Hide resolved

bpierre commented Aug 14, 2020

View reviewed changes

packages/connect-thegraph/src/core/GraphQLWrapper.ts Show resolved Hide resolved

bpierre commented Aug 14, 2020

View reviewed changes

packages/connect-thegraph/src/core/GraphQLWrapper.ts Outdated Show resolved Hide resolved

bpierre added 2 commits August 14, 2020 01:45

Align poll interval on the average block time

865441c

GraphQLWrapper: replace the operation type dynamically.

5a44409

This is so app connectors don’t have to change it to `query`, then back to ${type} whenever we will support GraphQL subscriptions again.

0xGabi reviewed Aug 14, 2020

View reviewed changes

packages/connect-thegraph/src/core/GraphQLWrapper.ts Outdated Show resolved Hide resolved

0xGabi approved these changes Aug 14, 2020

View reviewed changes

bpierre added 8 commits August 14, 2020 16:57

Add bundler-specific demos

64a0685

Two for now: - Parcel - Webpack with ts-loader

list-balances-cli: chainId => network

2b72d17

list-votes-cli: use an existing org name

9acd438

minimal-setup: fix Parcel paths

b4ae51d

org-viewer-{react,web}: move from webpack 5 to webpack 4

2c4eb56

Also remove the unecessary passing of The Graph URLs.

Update yarn.lock

d0e1c29

Update bundler-webpack-ts-loader app

0f5c05a

bundler-webpack-ts-loader: mainFields priority

973af14

bpierre added 2 commits August 14, 2020 18:30

Merge branch 'demos-cleanup' into poll-subscriptions

f105802

bpierre requested review from Evalir and 0xGabi August 15, 2020 17:50

Fix tests

99ffb8c

0xGabi reviewed Aug 16, 2020

View reviewed changes

packages/connect-voting-disputable/src/__test__/models/votes.test.ts Show resolved Hide resolved

bpierre added 4 commits August 17, 2020 12:09

Coding style

0d332cc

IOrganizationConnector: enforce an object as a config base

e78a90e

Coding style

4ee7a32

Fix TS union type

34ad85e

bpierre merged commit 1fff067 into master Aug 17, 2020

bpierre deleted the poll-subscriptions branch August 17, 2020 12:51

andy-hook mentioned this pull request Aug 20, 2020

React: Votes returning null inbetween poll updates #216

Closed

bpierre mentioned this pull request Aug 24, 2020

React API improvements #233

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace GraphQL subscriptions by queries + polling #203

Replace GraphQL subscriptions by queries + polling #203

bpierre commented Aug 14, 2020 •

edited

Loading

codecov bot commented Aug 14, 2020 •

edited

Loading

Evalir left a comment •

edited

Loading

0xGabi left a comment

bpierre commented Aug 14, 2020

0xGabi left a comment •

edited

Loading

0xGabi Aug 15, 2020

bpierre Aug 17, 2020

0xGabi Aug 16, 2020

bpierre Aug 17, 2020

0xGabi Aug 16, 2020

bpierre Aug 17, 2020

Replace GraphQL subscriptions by queries + polling #203

Replace GraphQL subscriptions by queries + polling #203

Conversation

bpierre commented Aug 14, 2020 • edited Loading

codecov bot commented Aug 14, 2020 • edited Loading

Codecov Report

Evalir left a comment • edited Loading

Choose a reason for hiding this comment

0xGabi left a comment

Choose a reason for hiding this comment

bpierre commented Aug 14, 2020

0xGabi left a comment • edited Loading

Choose a reason for hiding this comment

0xGabi Aug 15, 2020

Choose a reason for hiding this comment

bpierre Aug 17, 2020

Choose a reason for hiding this comment

0xGabi Aug 16, 2020

Choose a reason for hiding this comment

bpierre Aug 17, 2020

Choose a reason for hiding this comment

0xGabi Aug 16, 2020

Choose a reason for hiding this comment

bpierre Aug 17, 2020

Choose a reason for hiding this comment

bpierre commented Aug 14, 2020 •

edited

Loading

codecov bot commented Aug 14, 2020 •

edited

Loading

Evalir left a comment •

edited

Loading

0xGabi left a comment •

edited

Loading