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

Cache.modify types failing after upgrading to 3.8 #11171

Closed
Slessi opened this issue Aug 25, 2023 · 6 comments · Fixed by #11206
Closed

Cache.modify types failing after upgrading to 3.8 #11171

Slessi opened this issue Aug 25, 2023 · 6 comments · Fixed by #11206
Assignees

Comments

@Slessi
Copy link

Slessi commented Aug 25, 2023

Issue Description

This code I use to add an item to a list in the cache passes typechecking for me in 3.7, but does not in 3.8.
I suspect due to the changes in #10895

Type '(refs: Reference[], { toReference }: ModifierDetails) => Reference[]' is not assignable to type 'Modifier<readonly Reference[]>'.
  Types of parameters 'refs' and 'value' are incompatible.
    The type 'readonly Reference[]' is 'readonly' and cannot be assigned to the mutable type 'Reference[]'.ts(2322)
cache.modify({
  id: cache.identify(team),
  fields: {
    tracks(refs: Reference[], { toReference }) {
      const ref = toReference(track);
      if (!ref) return refs;

      return refs.concat(ref); // TS error here
    },
  },
})

If I try to make it work like the changes suggest by adding a generic here: cache.modify<T> and dropping the manual Reference[] type from the arguments, the error grows longer (but triggers on same line):

The interface I am supplying here was generated by graphql-codegen

Type '(refs: readonly Reference[] | readonly Track[], { toReference }: ModifierDetails) => readonly Reference[] | readonly Track[] | (Reference | Track)[]' is not assignable to type 'Modifier<readonly Reference[] | readonly Track[]>'.
  Type 'readonly Reference[] | readonly Track[] | (Reference | Track)[]' is not assignable to type 'readonly Reference[] | readonly Track[] | DeleteModifier | InvalidateModifier'.
    Type '(Reference | Track)[]' is not assignable to type 'readonly Reference[] | readonly Track[] | DeleteModifier | InvalidateModifier'.
      Type '(Reference | Track)[]' is not assignable to type 'readonly Track[]'.
        Type 'Reference | Track' is not assignable to type 'Track'.
          Type 'Reference' is missing the following properties from type 'Track': accessible, actual_pool_size, can_skip, challenges, and 55 more.ts(2322)

At the moment, the only way I can find to silence the error is to use no generic and no Reference[] (which effectively makes refs any again)

Link to Reproduction

https://codesandbox.io/s/gracious-ardinghelli-mv4h49?file=/src/index.tsx

Reproduction Steps

Click repro, check TS errors

@phryneas
Copy link
Member

phryneas commented Sep 11, 2023

Hi @Slessi!
I have addressed this over in #11206. Could you check out the CI build from that issue and give feedback if it fixes the type issues you have here?

npm i @apollo/[email protected]

@phryneas phryneas removed the 🏓 awaiting-team-response requires input from the apollo team label Sep 11, 2023
@Slessi
Copy link
Author

Slessi commented Sep 11, 2023

@phryneas there is no change with that PR, tried it locally and on codesandbox

Edit: Actually, if I change my code to use cache.modify<T>, its allowed now with your PR.

I still get readonly errors with this signature though of using refs:Reference[] (which used to work) but I could live with the new version.

cache.modify({
  id: cache.identify(team),
  fields: {
    tracks(refs: Reference[], { toReference }) {
      const ref = toReference(track);
      if (!ref) return refs;

      return refs.concat(ref); // TS error here
    },
  },
})

@phryneas
Copy link
Member

I still get readonly errors with this signature though of using refs:Reference[] (which used to work) but I could live with the new version.

Yeah, that was a part of the types that was just generally incorrect, we had to change that:
If you had data in that cache entry that could not be normalized (e.g. it doesn't have an id), you will get Entity[] there, not Reference[] - so there were two options for us: Reference[] | Entity[] (which was the former change), or (Reference | Entity)[] (which is the new, more permissive, version).

(We also cannot check for id and make it based on that, because you could have changed typePolicies so it actually uses something else for normalization...)

Unfortunately neither is compatible with the code you previously had, because it was ignoring the option of Entity there - but I'm happy to see that modify<T> opens a way forward for you now.

@saranshkataria
Copy link

Are there any plans to merge this PR anytime soon? modify<T> works for our project as well with this PR, but not without it.

@phryneas
Copy link
Member

I'm sorry - we're currently in the middle of a lot of conferences, so it's a lot of traveling right now.
I'll put it on the agenda for our team meeting in two weeks, so I hope we'll get it reviewed then.

Copy link
Contributor

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
For general questions, we recommend using StackOverflow or our discord server.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants