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

Query doesn't return empty entries from cached array (offset pagination) any more #6628

Open
cbergmiller opened this issue Jul 17, 2020 · 15 comments

Comments

@cbergmiller
Copy link

We noticed that our implementation of an offset/limit bases pagination broke. This used to work with earlier Apollo Client 3.0 versions (beta/rc) but doesn't work in 3.0.2.

Intended outcome:
Custom type policies should be able to merge pagination results . useQuery should return the array with the empty entries so that the component can find the data it needs to display a page based on an offset and limit.

Actual outcome:
The cache shows an array with empty entries. useQuery returns the array with the empty items removed.

Apollo Client 3.0.2
React 16.13.1

@cbergmiller cbergmiller changed the title Quries doesn't return empty entries from cached array (offset pagination) any more Query doesn't return empty entries from cached array (offset pagination) any more Jul 17, 2020
@benjamn
Copy link
Member

benjamn commented Jul 17, 2020

This is an intentional consequence of #6454, but you can opt out of this filtering behavior by replacing the empty items with null as a placeholder, when returning a given array from your read function. I realize this placeholder trick is not well documented, but it does have tests, so I'm confident it will (continue to) work.

Depending on what exactly you mean by "empty" (dangling references? empty objects? undefined?), the options.canRead(item) helper (#6425) may be useful to determine if a given item is considered readable by the cache:

new InMemoryCache({
  typePolicies: {
    Person: {
      fields: {
        friends(existing, { args, canRead }) {
          // ... logic to produce a slice of existing based on args...

          // Using a null placeholder will ensure the slice retains its original length,
          // effectively disabling the default filtering of unreadable items:
          return slice.map(friend => canRead(friend) ? friend : null);

          // By default, the cache behaves as if you returned the following array:
          // return slice.filter(canRead);
        },
      },
    },
  },
})

@cbergmiller
Copy link
Author

Thanks for the quick response!
By empty i meant:

const arr = [];
arr[1] = "Foo";

arr will now be [empty, "Foo"]

We used this to merge arbitrary pages in the cache.

The type policy for reference:

function makeOffsetPaginationPolicy(): TypePolicy {
    return {
        fields: {
            rows: {
                keyArgs: false,
                merge(existing: any[], incoming: any[], params) {
                    const offset = params.variables.offset;
                    const merged = existing ? existing.slice(0) : [];
                    // Insert the incoming elements in the right places, according to args.
                    const end = offset + incoming.length;
                    for (let i = offset; i < end; ++i) {
                        merged[i] = incoming[i - offset];
                    }
                    return merged;
                },
            },
        },
    };
}

@cbergmiller
Copy link
Author

I fixed this in our code by writing null in the cache and with a field read function:

/**
 * Create TypePolicy for offset/limit based pagination.
 */
function makeOffsetPaginationPolicy(): TypePolicy {
    return {
        fields: {
            rows: {
                keyArgs: false,
                merge(existing: any[], incoming: any[], params) {
                    const offset = params.variables.offset;
                    const merged = existing ? existing.slice(0) : [];
                    // Insert the incoming elements in the right places, according to args.
                    const newEnd = offset + incoming.length;
                    const oldEnd = existing?.length ?? 0;
                    for (let i = Math.min(offset, oldEnd); i < newEnd; ++i) {
                        if (i >= offset) {
                            merged[i] = incoming[i - offset];
                        } else if (i >= oldEnd) {
                            // add placeholder
                            merged[i] = null;
                        }
                    }
                    return merged;
                },
                read(existing) {
                    return existing ?? [];
                },
            },
        },
    };
}

@transcranial
Copy link

The provided offsetLimitPagination policy no longer works properly as it creates sparse arrays that are now automatically filtered: [0, 1, 2, <3 empty items>, 6] becomes [0, 1, 2, 6].

@vigie
Copy link

vigie commented Aug 22, 2020

Exact same problem here. I was relying on sparse arrays to work with offset based pagination and it was working great. I really don't want to pad the array with potentially thousands of nulls as a workaround. And as @transcranial points out above, based on the fact that the provided offsetLimitPagination utility also relies on sparse arrays, this does seem to be a bug in core.

This issue is preventing me from moving past the RC and onto the official release of 3. Is there a sane way we can reintroduce support for sparse arrays for the purposes of offset pagination?

@vigie
Copy link

vigie commented Sep 2, 2020

Further notes...the fix as suggested by @benjamn will not work in the case of sparse arrays (including the officially provided offsetLimitPagintation util) as the callback of a map function is not invoked for a sparse array "hole" (https://remysharp.com/2018/06/26/an-adventure-in-sparse-arrays).

@cbergmiller workaround does work, but does suffer from the padding problem I mentioned in my previous comment, so does still feel to be like a workaround rather than an acceptable solution.

@hwillson
Copy link
Member

hwillson commented May 4, 2021

Let us know if this is still a concern with @apollo/client@latest - thanks!

@hwillson hwillson closed this as completed May 4, 2021
@vigie
Copy link

vigie commented May 4, 2021

Hi @hwillson it is still a problem with latest. It would require an intentional update to reintroduce support for storing sparse arrays in Apollo cache. The current implementation uses a filter array operator that reduces a sparse array to a non-sparse array before storing.

The downside to no longer supporting sparse arrays, as previously noted, is that it forces clients to pad their arrays with potentially millions of nulls before storing (imagine the case where only the first and thousandth page of data had been fetched, for example).

@hwillson hwillson reopened this May 5, 2021
@levrik
Copy link
Contributor

levrik commented Nov 29, 2021

Basically this currently blocks us from implementing server-side pagination. But without our UI just doesn't load anymore as requests are running into timeouts. Any other solutions besides filling everything with null? This doesn't sound very performant to me.

@cbergmiller
Copy link
Author

This may not help your specific case but i moved to cursor-based pagination. Less effort in the frontend and better perfoming DB queries (large offsets tended to be slow).

@levrik
Copy link
Contributor

levrik commented Dec 2, 2021

@cbergmiller Also already thought about it but we have server-side sorting and I've heard that this is getting tricky with cursor-based pagination.

@adamyonk
Copy link
Contributor

adamyonk commented Oct 7, 2022

@benjamn @hwillson I think this should be clarified in the docs, you still have to patch the read to make sure it doesn't blow away empty slots in the array. Additionally, you can't map over sparse arrays like this:

slice.map(friend => canRead(friend) ? friend : null);

map will leave empty slots in the resulting array, but it does not call the callback function on them. To actually turn the empty slots into null, you have to loop with a for:

for (let i = 0; i < slice.length; ++i) {
  slice[i] = canRead(slice[i]) ? slice[i] : null;
}

@bignimbus
Copy link
Contributor

@adamyonk thanks for the suggestion! Would you be up for suggesting a change to the docs via a Pull Request?

And thanks to all in this thread for sharing your concerns with this approach. We won't be able to prioritize revisiting this aspect of the code in the near future but will keep this issue open for any documentation updates that might ship as a result 🙏🏻

@rmtobin
Copy link

rmtobin commented May 17, 2023

I just ran into this issue again while attempting to reconfigure pagination in my app. Respectfully, the provided offsetLimitPagination utility and documentation should be removed if it is not going to be updated as it simply does not work as the docs describe and has not worked for almost 3 years. It is quite frustrating to follow documentation exactly and have different results - something that over time leads to lost confidence in Apollo overall.

@ettoredn
Copy link

ettoredn commented May 22, 2023

The following should work as expected for a limit-offset type of pagination. It leverages the fact that the cache preserves sparse arrays.

import {offsetLimitPagination as offsetLimitPaginationOriginal} from "@apollo/client/utilities";

const offsetLimitPagination: typeof offsetLimitPaginationOriginal = (keyArgs = false) => {
  return {
    ...offsetLimitPaginationOriginal(),
    read(existing, {args, canRead}) {
      // console.debug(`Reading from cache offset=${args?.offset} limit=${args?.limit} existing=`, existing);
      if (!Array.isArray(existing) || !Number.isFinite(args?.limit)) return undefined; // pass-through non-paginated queries

      const {offset = 0, limit} = args;
      const slice = existing?.slice(offset, offset + limit).filter(Object); // Filter out empty values from the sparse array

      return !slice.length ? undefined : slice;
    }
  }
}

  cache: new InMemoryCache({
    typePolicies: {
      Query: {
        fields: {
          users: {
            ...offsetLimitPagination(),
          },
        }
      }
    }
  }),

React component:

const [pageInfo, setPageInfo] = useState<{offset: number; limit: number;}>({offset: 0, limit: 10});
const {data: usersDTO} = useQuery(UsersPaginate, { variables: {
...pageInfo
}})

const loadPage = (page: number) => {
  const offset = pageInfo.limit * (page-1);
  setPageInfo({...pageInfo, offset}); // triggers refetch and field policy's read (see client.ts)
}
....

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

No branches or pull requests

10 participants