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

Add type/field policy code splitting docs section #6766

Closed
wants to merge 3 commits into from

Conversation

hwillson
Copy link
Member

@hwillson hwillson commented Aug 3, 2020

Demonstrate how to use cache.policies.addTypePolicies() to lazily load cache type/field policies.

Reference: #6711

Demonstrate how to use `cache.policies.addTypePolicies()` to
lazily load cache type/field policies.
@hwillson hwillson requested a review from StephenBarlow as a code owner August 3, 2020 14:58
@hwillson hwillson requested review from benjamn and jcreighton August 3, 2020 14:58
@hwillson hwillson self-assigned this Aug 3, 2020
@hwillson hwillson added this to the Post 3.0 milestone Aug 3, 2020
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! I caught a couple things I think we should fix, just so people don't copy/paste dodgy patterns.

docs/source/caching/advanced-topics.mdx Outdated Show resolved Hide resolved
docs/source/caching/advanced-topics.mdx Outdated Show resolved Hide resolved
Comment on lines +415 to +417
export function Stats() {
const client = useApolloClient();
client.cache.policies.addTypePolicies(newPolicy);
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to avoid re-adding the policies every time the component is rendered. I realize the component function gives us access to the client/cache, but maybe we can come up with a better pattern here? Maybe keep a Set of cache instances that we've seen, and only add the policies when we haven't seen this cache before?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing @benjamn, I'l adjust. Thanks!

Copy link

@ashubham ashubham Oct 7, 2020

Choose a reason for hiding this comment

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

Would be great to clarify the best practice here. We could use useEffect with no dependencies too.

Copy link

Choose a reason for hiding this comment

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

I wonder how that relates to this comment (that the query gets fired before to policies are loaded, unless you use a lazy query).

Copy link

Choose a reason for hiding this comment

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

Thanks for the link @coler-j, the effect may be the reason why I was seeing the failure in the issue I opened (as you linked). This is our hook that adds type policies: https://github.com/magento/pwa-studio/blob/develop/packages/peregrine/lib/hooks/useTypePolicies.js#L29.

const client = useApolloClient();
client.cache.policies.addTypePolicies(newPolicy);

const { loading, data: { messageCount } } = useQuery(GET_MESSAGE_COUNT);
Copy link
Member

Choose a reason for hiding this comment

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

Since data can be undefined, we either need to avoid destructuring it, or provide a default = {} expression.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll fix @benjamn - thanks!


## Code splitting

Depending on the complexity and size of your cache type/field policies, you might not always want to define them up front, when you create your initial `InMemoryCache` instance. If you have type/field policies that are only needed in a specific part of your application, you can leverage `InMemoryCache`'s `.policies.addTypePolicies()` method to adjust your cache policy map at any point. This can be really useful when leveraging techniques like route based code-splitting, using a tool like [`react-loadable`](https://www.npmjs.com/package/react-loadable).
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend documenting "Loadable Components" instead since react-loadable is more or less unmaintained and the official React docs have moved on with their suggestion: https://reactjs.org/docs/code-splitting.html#reactlazy

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point @bwhitty! These docs were carried over / modified from the AC 2.x docs, so they definitely need to be updated further. Thanks!

@bennypowers
Copy link
Contributor

I'm documenting this for apollo-elements at https://apolloelements.dev/pages/cool%20tricks/code-splitting, FYI. Thanks for working on this :D

@bennypowers
Copy link
Contributor

I noticed that while policies is public on InMemoryCache, it doesn't exist on ApolloCache. Does it make sense to add policies?: Policies to the ApolloCache interface?

Otherwise, one would have to
as ApolloClient<NormalizedCacheObject> & { cache: InMemoryCache }

@salmanalfariz24
Copy link

Hi @hwillson, any plan to include this in the docs soon?

@jpvajda jpvajda added the 🏓 awaiting-team-response requires input from the apollo team label May 5, 2022
@jpvajda
Copy link
Contributor

jpvajda commented May 5, 2022

@hwillson did you want to come back to this documentation PR so we can merge it in?

@hwillson hwillson removed this from the Post 3.0 milestone Aug 11, 2022
@alessbell
Copy link
Contributor

Thanks for the discussion, everyone! We may want to revisit this in the future, but the existing draft would need significant reworking given the changes to the React landscape in the time that's passed, so I'm going to close this out for now. Thanks 🙇‍♀️

@alessbell alessbell closed this Feb 2, 2023
@apollographql apollographql locked and limited conversation to collaborators Feb 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🏓 awaiting-team-response requires input from the apollo team 📝 documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants