Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(experimental): graphql: add multipleAccounts root query #2587

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

buffalojoec
Copy link
Collaborator

@buffalojoec buffalojoec commented May 1, 2024

Problem

The GraphQL resolver currently supports root queries for account and
programAccounts, but not for multipleAccounts. A user can craft multiple
account queries in one source, but why go through the trouble if all fields and
input parameters are the same?

Additionally, if the GraphQL resolver had a resolveMultipleAccounts resolver
function, it would make things much easier to add list-based account lookups
on schema types that would otherwise return [Address]. With this new resolver,
they can instead return [Account], which would allow nested queries on these
lists.

Summary of Changes

Introduce the mutipleAccounts root query.

I've reimplemented the same tests for this new multipleAccounts query
as those already provided for the existing accounts query.

  • src/__tests__/account-test.ts
  • src/loaders/__tests__/account-loader-test.ts

ie:

  • src/__tests__/multiple-accounts-test.ts
  • src/loaders/__tests__/multiple-accounts-loader-test.ts

Copy link

changeset-bot bot commented May 1, 2024

⚠️ No Changeset found

Latest commit: 4efc863

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @buffalojoec and the rest of your teammates on Graphite Graphite

@buffalojoec buffalojoec marked this pull request as ready for review May 1, 2024 00:02
Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

I think there are probably a number of good and perhaps surprising reasons not to make a plural field version of this, but I’ll have to spend some time explaining why. I’ll get to it tomorrow.

Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

Alright, so let's talk about plural fields.

Let's break down the use cases for multiple account fetches.

Use cases

Case 1: You know the accounts statically

If your app straight up knows which accounts it needs, either literally statically (the addresses are constants in the code) or via arguments (there are always n accounts it needs, and the addresses of those will be supplied as inputs to the application) then it should declare them each in the query.

const data = useFragment(graphql`
    fragment Home_query on Query {
        userProfileAccount: account(address: $userProfileAccountAddress) {
            data
        }
        gameStateAccount: account(address: $gameStateAccountAddress) {
            data
        }
    }
`, props.query);

Pros

  • Each account() fetch can declare its own arguments (eg. commitment) independent of the others.
  • Each account can be refetched independently.

Case 2: You need an unknown number of accounts, you don't have the addresses, but you know the root

In this case, the schema usually offers a field (consider walletAccounts in the example below) that resolves zero-or-more objects given some context/parent.

const data = useFragment(graphql`
    fragment UserProfile_user on User {
        walletAccounts(dataSlice: { length: 0, offset: 0 }) {
            ...Wallet_account
            address
        }
    }
`, props.user);
return (
    <ul>
        {data.walletAccounts.map(account => 
            <Wallet
                account={account}
                key={account.address}
            />
        }
    </ul>
);

Pros

  • You need not know the addresses of the entities up front to be able to fetch zero-or-more of them

Cons

  • The entire fetch for the list of accounts must share the same arguments (eg. dataSlice).
  • You get all of the entities over the wire, whether your app has space to render them or not. Plural fields offer no way to paginate.

Case 3: You need a dynamic list of accounts for which you have the addresses

Consider an app that lets the user check the balance of multiple accounts, by literally supplying the accounts.

You could do this:

const [data, refetch] = useRefetchableFragment(graphql`
    fragment AccountBalances_query on Query
        @refetchable(queryName: "AccountBalancesRefetchQuery") {
        accounts(accounts: $accounts) {
            ...Balance_account
            address
        }
    }
`, props.query);
return (
    <>
        <textarea onChange={(e) => {
            const addresses = e.target.value.split('\n');
            refetch({ accounts:  addresses });
        }}></textarea>
        <ul>
            {data.accounts.map(account =>
                <Balance
                    account={account}
                    key={account.address}
                />
            }
        </ul>
    </>
);

Cons

  • Every time an address is added or removed, the entire account list gets fetched again, even the ones you already have locally.
  • The entire fetch for the list of accounts must share the same arguments (eg. commitment).

Discussion

  1. The only use case that requires a plural root field is case 3.
  2. Case 3 can be implemented using a dynamic set of entrypoints instead of a single page that queries a plural root field. Imagine each result UI being, itself, a mini-page based on the address as input. When the user adds or removes an address to their list, a new entrypoint is fetched and rendered, but the others don't change. Think of entrypoints as islands of one or more root queries that make up a larger app, and share data with the entire app.

@@ -2,6 +2,7 @@ export const rootTypeDefs = /* GraphQL */ `
type Query {
account(address: Address!, commitment: Commitment, minContextSlot: Slot): Account
block(slot: Slot!, commitment: CommitmentWithoutProcessed): Block
multipleAccounts(addresses: [Address!]!, commitment: Commitment, minContextSlot: Slot): [Account]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually just follow pluralization rules.

Suggested change
multipleAccounts(addresses: [Address!]!, commitment: Commitment, minContextSlot: Slot): [Account]
accounts(addresses: [Address!]!, commitment: Commitment, minContextSlot: Slot): [Account]

@@ -2,6 +2,7 @@ export const rootTypeDefs = /* GraphQL */ `
type Query {
account(address: Address!, commitment: Commitment, minContextSlot: Slot): Account
block(slot: Slot!, commitment: CommitmentWithoutProcessed): Block
multipleAccounts(addresses: [Address!]!, commitment: Commitment, minContextSlot: Slot): [Account]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This field, as written, returns a nullable array. This will force the consumer to treat null and [] the same with two different checks. What you actually want is to unconditionally return an array, maybe empty, but in any case with null for every unfetchable account.

Suggested change
multipleAccounts(addresses: [Address!]!, commitment: Commitment, minContextSlot: Slot): [Account]
multipleAccounts(addresses: [Address!]!, commitment: Commitment, minContextSlot: Slot): [Account]!

@buffalojoec
Copy link
Collaborator Author

buffalojoec commented May 14, 2024

@steveluscher Thanks for the detailed breakdown. It sounds to me like this is probably not The Way™. I'm mostly fixated on the fact that case 3 is the most valid use for such a query, and the cons I think are too much to bear.

Admittedly, I viewed Case3: Con 2 as a potential Pro, but now I'm starting to see how it could very easily become either one.

I think I may abandon this change.

With that being said, I was open to the idea of at least introducing the resolveMultipleAccounts resolver, to populate fields that would otherwise be lists of addresses, but even going down that road seems like it might wind us up in the same kind of boat as Case 3: Con 1.

I'll look more into pagination and connections before circling back to this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants