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

docs: fixup onResetStore docs, convert to hooks #10047

Merged

Conversation

henryqdineen
Copy link
Contributor

Found a couple issues with the existing onResetStore docs

  • componentDidUnmount doesn't exists (componentWillUnmount does though)
  • this.state.reset is always false and wasn't correctly being set
  • It references the old @apollo/react-hoc module from react-apollo, which has not been mentioned if reading the docs in order

For this PR I also migrated to hooks since the HOCs are deprecate. Let me know if you would prefer it to be fixed up but still use the HOC. Thanks!

@henryqdineen henryqdineen changed the title fixup onResetStore docs, convert to hooks docs: fixup onResetStore docs, convert to hooks Aug 28, 2022
@jpvajda jpvajda self-assigned this Sep 1, 2022
Copy link
Contributor

@jpvajda jpvajda 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 this contribution! I noticed up the page (on line 49) we have another code sample which does use withApollo

export default withApollo(graphql(PROFILE_QUERY, {
  props: ({ data: { loading, currentUser }, ownProps: { client }}) => ({
    loading,
    currentUser,
    resetOnLogout: async () => client.resetStore(),
  }),
})(Profile));

We can probably update that as well in this PR, did you want to make that change?

@jpvajda jpvajda added the 🏓 awaiting-contributor-response requires input from a contributor label Sep 1, 2022
@henryqdineen
Copy link
Contributor Author

henryqdineen commented Sep 1, 2022

Thanks @jpvajda. I'm not super familiar with the graphql HOC. Would something like this work? I can update this branch.

export default function Profile() {
  const { data, loading, client } = useQuery(PROFILE_QUERY);

  return (
    <ProfileDisplay
      loading={loading}
      currentUser={data?.currentUser}
      resetOnLogout={async () => client.resetStore()}
    />
  );
}

@jpvajda
Copy link
Contributor

jpvajda commented Sep 1, 2022

@henryqdineen good question! I'll defer to the team on that one 😄 . cc @benjamn @hwillson @MrDoomBringer @alessbell

@jpvajda jpvajda removed their assignment Sep 6, 2022
@MrDoomBringer MrDoomBringer self-assigned this Sep 12, 2022
Copy link
Contributor

@MrDoomBringer MrDoomBringer 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 the PR @henryqdineen, I have some small suggestions but other than that this looks great!

docs/source/caching/advanced-topics.mdx Outdated Show resolved Hide resolved
@MrDoomBringer
Copy link
Contributor

MrDoomBringer commented Sep 12, 2022

Would something like this work? I can update this branch.

@henryqdineen Not a blocking request, but it might be better to use something like this, purely because it could be a little easier for people to drop-in to existing code.

function Profile() {
    const { data, client } = useQuery(PROFILE_QUERY);
    return (
      <Fragment>
        <p>Current user: {data?.currentUser}</p>
        <button onClick={async ()=>client.resetStore()}>
            Log out
        </button>
      </Fragment>
    );
}

All the best,
Emmanuel, Intern :-)

Co-authored-by: Emmanuel S. <emmanuelssr@gmail.com>
@jpvajda jpvajda added 🏓 awaiting-team-response requires input from the apollo team and removed 🏓 awaiting-contributor-response requires input from a contributor labels Sep 13, 2022
@MrDoomBringer
Copy link
Contributor

LGTM! @henryqdineen I went ahead and committed an update to that first example for client.resetStore, as it looked like you were alright with the suggestion. Thanks for the other changes.

All the best,
Emmanuel, Intern :-)

@MrDoomBringer MrDoomBringer merged commit 11c528b into apollographql:main Sep 19, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 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.

3 participants