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

addTypePolicies does not work as expected for local fields #378

Open
sirugh opened this issue Mar 25, 2021 · 10 comments
Open

addTypePolicies does not work as expected for local fields #378

sirugh opened this issue Mar 25, 2021 · 10 comments
Labels
📕 cache Feature requests related to the cache

Comments

@sirugh
Copy link

sirugh commented Mar 25, 2021

Intended outcome:

Type Policies for local state fields added through addTypePolicies at render/mount of component should allow for querying of those fields from that point on.

Actual outcome:

Type Policies for local state fields added through addTypePolicies at render/mount of component return undefined data and no errors. loading state indicates that the query executed.

Note: I dug into the addTypePolicies call and for both scenarios it seems like the type is added. The image shows the same resulting state for adding policy via initialization or adding policy via component calling addTypePolicies.

How to reproduce the issue:

See sandbox: https://codesandbox.io/s/local-query-typepolicies-bug-s2ssq?file=/src/index.js

Instructions within, but at a high level - it seems to be the difference between applying type policies to the cache at creation vs applying type policies at a later time. I have tried to narrow down the sandbox as much as I could.

// This works and `message` is queryable.
const cache = new InMemoryCache({
    typePolicies: {
        Query: {
            fields: {
                message: {
                    read() {
                        return 'Hello World';
                    }
                }
            }
        }
    }
});

vs

// This doesn't work and `message` returns undefined.
const MyComponent = () => {
  const apolloClient = useApolloClient();
  useEffect(() => {
    apolloClient.cache.policies.addTypePolicies({
        Query: {
            fields: {
                message: {
                    read() {
                        return 'Hello World';
                    }
                }
            }
        }
    });
  }, [apolloClient, typePolicies]);
}

Versions
Image from Gyazo

@sirugh
Copy link
Author

sirugh commented Mar 25, 2021

Wow now that I spent all that effort I think the issue is that we were not waiting to call useQuery. So it ran before the effect for adding the policy. This worked:

useTypePolicies(DEFAULT_TYPES);

  const [runQuery, { data, loading, error }] = useLazyQuery(GET_TEXT);

  useEffect(() => {
    runQuery();
  }, [runQuery]);

@coler-j
Copy link

coler-j commented Mar 25, 2021

So if you want to load type policies via addTypePolicies you MUST only use lazy queries?
I don't see any docs about addTypePolicies, where did you find it? I was previously using addResolvers (for local resolvers) and didn't need to use lazy queries.

@sirugh
Copy link
Author

sirugh commented Mar 25, 2021

So if you want to load type policies via addTypePolicies you MUST only use lazy queries?

Yes, I believe so, as addTypePolicies does not return anything (it adds the policy to a queue which is then processed at some point). If there was some way to observe the change and ensure the policy was added we could wrap the call with a promise or something.

I don't see any docs about addTypePolicies, where did you find it?

I think resolvers/addResolvers for local fields is being deprecated and I had asked @benjamn about a replacement for runtime resolver additions during the last GQL conf. His answer was this function, which is undocumented, and lives at apolloClient.cache.policies.addTypePolicies.

@coler-j
Copy link

coler-j commented Mar 25, 2021

Thanks @sirugh I just found this PR that adds docs, but it is out of date. apollographql/apollo-client#6766

@benjamn
Copy link
Member

benjamn commented Mar 25, 2021

Please note that it's not possible to go back in time whenever addTypePolicies is called, and rewrite the cache as if those type policies had been in place the whole time. The point in time when type policies are defined is crucially important, for the sake of cache consistency/coherence.

In my mind, this reality implies you should either add all your type policies at application initialization time (probably just using the new InMemoryCache({ typePolicies }) option), or be very careful to use addTypePolicies only when you know there's no chance those types have been used earlier (before the relevant policies were added).

In other words, I want to underscore @sirugh's finding that the order of addTypePolicies and useQuery definitely does matter, but I also want to recommend adding the type policies (much) earlier, even if it means not code-splitting them.

@sirugh
Copy link
Author

sirugh commented Mar 25, 2021

Thanks for the quick response @benjamn. I'll relay this to our team.

Could you clarify what you mean by this:

be very careful to use addTypePolicies only when you know there's no chance those types have been used earlier (before the relevant policies were added).

What do you mean by "used"? Our use case is the ability for folks to arbitrarily inject, at runtime, custom types. Like a plugin or extension point.

Lastly, do you have any advice for how one might be alerted to the successful addition of a policy via addTypePolicies?

@benjamn
Copy link
Member

benjamn commented Mar 25, 2021

Implementation-wise, there's a private policies.getTypePolicy(typename: string) method that gets called whenever the cache needs to look up the policy for a given typename string.

Although you could maybe get away with adding a few more fields to a type policy after getTypePolicy has been called for that type (depending on whether you've ever read or written a query with those fields before), you're in shaky territory at that point. If we wanted to enforce a rule that guarantees safety, I would vote for: adding new type policies with addTypePolicies throws an error if any of those types have already been looked up with getTypePolicy.

@sirugh Would that be a useful rule to enforce, in your case?

@sirugh
Copy link
Author

sirugh commented Mar 25, 2021

Is it guaranteed to fail if you call addTypePolicies with fields that have already been queried for previously? I'm all for protecting against cases that don't work.

@benjamn
Copy link
Member

benjamn commented Mar 25, 2021

It's not currently guaranteed to fail, but I'm in favor of making addTypePolicies fail in that case.

@benjamn benjamn self-assigned this Mar 25, 2021
@sirugh
Copy link
Author

sirugh commented Mar 26, 2021

Sounds like a breaking change - which would be fine so long as you let me know about it via a major bump or just...tell me :)

@alessbell alessbell transferred this issue from apollographql/apollo-client Apr 28, 2023
@alessbell alessbell added the project-apollo-client (legacy) LEGACY TAG DO NOT USE label Apr 28, 2023
@jerelmiller jerelmiller added 📕 cache Feature requests related to the cache and removed project-apollo-client (legacy) LEGACY TAG DO NOT USE labels Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📕 cache Feature requests related to the cache
Projects
None yet
Development

No branches or pull requests

5 participants