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

Drops --legacy-peer-deps from #4829 #4840

Open
wants to merge 52 commits into
base: develop
Choose a base branch
from
Open

Conversation

robertu7
Copy link
Contributor

@robertu7 robertu7 commented Sep 3, 2024

PR from #4829 by @lizhineng

@robertu7 robertu7 requested review from wlliaml and gitwoz September 3, 2024 09:53
@robertu7 robertu7 linked an issue Sep 3, 2024 that may be closed by this pull request
@gitwoz gitwoz requested review from guoliu, zeckli and gary02 September 3, 2024 10:00
@robertu7
Copy link
Contributor Author

robertu7 commented Sep 4, 2024

@lizhineng you can try to preview here: https://matters-web-git-fix-peer-dependency-matters.vercel.app/

I found there are some issue on local cache of article page. You can reproduce:

  1. Open the homepage;
  2. Click on any article;
  3. Click on any link in the article page;
  4. Go back to article page: expected not to show the loading state;

@lizhineng
Copy link
Contributor

The article refresh you’re seeing is actually expected. The useEffect hook runs whenever the component gets mounted back to the page.

In Apollo Client v2, the loading state didn’t visibly update, but the HTTP requests were still happening behind the scenes. I’ve recorded a video showing how this works in production for reference.

matters-article-refresh.mov

Here's a PR that applies a quick fix for the "issue." I don’t have a complete understanding of the business logic, so feel free to close this if it doesn't work as expected or causes any problems.

@robertu7
Copy link
Contributor Author

robertu7 commented Sep 6, 2024

Thanks for your debugging, i think this bug it's brought a fix from #3904, however, it's not useful anymore.

I don’t have a complete understanding of the business logic, so feel free to close this if it doesn't work as expected or causes any problems.

We still need the useEffect hook to fetch the "private data" to ensure that the data and rendering are correct for the current user and article state. We will close your PR and push the commit later.

@robertu7 robertu7 removed the request for review from wlliaml September 9, 2024 08:55
@robertu7
Copy link
Contributor Author

image

refact: auto-generated mergeable types
@lizhineng
Copy link
Contributor

image

Is this error caused by these PR changes? How do you reproduce it and maybe I can look it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump deps: Apollo Client 3.x
3 participants