-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 a "memory management" documentation page #11415
Conversation
|
size-limit report 📦
|
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
df0a619
to
5c6e6af
Compare
); | ||
const dist = path.resolve(__dirname, "../dist"); | ||
const entryPoints = map((entryPoint: { dirs: string[] }) => { | ||
return `export * from "${dist}/${entryPoint.dirs.join("/")}/index.d.ts";`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CacheSizes
interface is not part of the normal @apollo/client
entrypoint, so until now that was not part of the DocModel. This change creates a DocModel for all entrypoints combined.
It also moves some logic out as code was getting a bit confusing when everything was inlined down in buildReport
dbfb91a
to
1a7fbd2
Compare
1a7fbd2
to
21f8eea
Compare
@Meschreiber @alessbell @jerelmiller could I please get some feedback on this one? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beyond inline comments I would recommend removing the top H2 "Cache sizes" and bumping up all the subheader sizes (H3 -> H2, H4 -> H3).
"Cache sizes" applies to the entire document and should be mentioned in the page subtitle, and bumping up H4 -> H3 gives readers better visibility on the TOC.
Co-authored-by: Maria Elisabeth Schreiber <[email protected]>
Co-authored-by: Maria Elisabeth Schreiber <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caught a few minor things that needed changed for correctness, otherwise looks great! If @Meschreiber is happy, so am I!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the additional copy suggestions, @jerelmiller ! I also agree with the recommendation around using the minVersion
component in addition to the front matter so didn't commit that suggestion directly. I've added one more clarification, otherwise, LGTM!
Co-authored-by: Maria Elisabeth Schreiber <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Awesome stuff!
I still need to incorporate all the feedback on Docblock comments from #11408 into this one.The page can be reviewed here: https://deploy-preview-11415--apollo-client-docs.netlify.app/caching/memory-management
Checklist: