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

Update Home Page content after cache hit (#2230) #2453

Merged
merged 12 commits into from
Jun 12, 2020

Conversation

davemacaulay
Copy link
Contributor

@davemacaulay davemacaulay commented Jun 2, 2020

Description

Apollo will not query the GraphQL endpoint for a new version of CMS Page content due to the usage of the default fetch policy, cache only. This PR updates this cache policy to use cache-and-network (https://www.apollographql.com/docs/react/api/react-apollo/#optionsfetchpolicy) which will ensure the cached data is rendered, then a request is made to retrieve any new data if available.

This fetch policy ensures a quick user experience while ensuring up to date content is rendered on the homepage. If there was a change in content between the cache and GraphQL Apollo automatically handles re-rendering this.

Related Issue

Closes #2230 .
https://jira.corp.magento.com/browse/PWA-471

Acceptance

Verification Stakeholders

@jimbo
@sirugh
@brendanfalkowski
@dhaecker

Specification

https://www.apollographql.com/docs/react/api/react-apollo/#optionsfetchpolicy

Verification Steps

  • Navigate to homepage in app (allowing content to be cached)
  • Open Admin
  • Go to Content > Pages > Home page
  • Edit the home page and save
  • Refresh the application (the old content will display, which is expected for the first reload)
  • Refresh the application again
  • The old content will be displayed again, as it will for any number of times that the page is refreshed
  • Manually clear the localStorage property, {{apollo-cache-persist}}
  • Refresh the application
  • Now the updated home page data will be displayed

Screenshots / Screen Captures (if appropriate)

2020-06-02 16 24 40

New Experience:
2020-06-12 10 06 55

Checklist

  • I have added tests to cover my changes, if necessary.
  • I have updated the documentation accordingly, if necessary.

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Jun 2, 2020

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against 239d8d9

@devops-pwa-codebuild
Copy link
Collaborator

devops-pwa-codebuild commented Jun 2, 2020

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2453.pwa-venia.com : LH Performance Expected 0.85 Actual 0.54, LH Best Practices Expected 1 Actual 0.92
https://pr-2453.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.35, LH Best Practices Expected 1 Actual 0.92
https://pr-2453.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.49, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92

- Remove redundant loading flag and null response on no data
- Add page loading state to app context
- Add loading icon into header
- Trigger page loading from CMS root component
@davemacaulay davemacaulay added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Jun 9, 2020
@davemacaulay davemacaulay marked this pull request as ready for review June 10, 2020 20:28
@zetlen
Copy link
Contributor

zetlen commented Jun 10, 2020

Is PageLoadingIndicator something different from packages/venia-ui/lib/components/LoadingIndicator or is it a reimplementation?

This is a well-designed API and hook for page load. We already have that concept in the codebase, but it's not perfect. Did you look at LoadingIndicator first, or just make a simple PageLoadingIndicator real fast?

@davemacaulay
Copy link
Contributor Author

davemacaulay commented Jun 10, 2020

Is PageLoadingIndicator something different from packages/venia-ui/lib/components/LoadingIndicator or is it a reimplementation?

This is a well-designed API and hook for page load. We already have that concept in the codebase, but it's not perfect. Did you look at LoadingIndicator first, or just make a simple PageLoadingIndicator real fast?

@zetlen they have very different purposes. The LoadingIndicator is used to signify the whole app is loading and is purposefully blocking. The new PageLoadingIndicator sits in the header and only represents a page is doing a cache-and-network load. It will only be displayed when Apollo is making that network request (while having already displayed the cached data), but might not actually change any data.

Copy link
Contributor

@jimbo jimbo left a comment

Choose a reason for hiding this comment

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

This is looking good. I've requested changes in a few places, but nothing significant.

packages/peregrine/lib/store/actions/app/asyncActions.js Outdated Show resolved Hide resolved
packages/peregrine/lib/store/reducers/app.js Outdated Show resolved Hide resolved
packages/peregrine/lib/talons/Header/useHeader.js Outdated Show resolved Hide resolved
packages/venia-ui/lib/RootComponents/CMS/cms.js Outdated Show resolved Hide resolved
packages/venia-ui/lib/components/Header/header.js Outdated Show resolved Hide resolved
packages/venia-ui/lib/components/Header/header.css Outdated Show resolved Hide resolved
@m2-community-project m2-community-project bot moved this from Ready for Review to Changes Requested in Pull Request Progress Jun 10, 2020
@jimbo
Copy link
Contributor

jimbo commented Jun 10, 2020

Did you look at LoadingIndicator first, or just make a simple PageLoadingIndicator real fast?

This is essentially a leaf-node component, and I'm okay if those proliferate. It's a thin wrapper around Icon; I'm afraid of getting too much more abstract than that. 😅

Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

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

One minor suggestion in addition to what's already requested. We need to have a team discussion about redux, but not a blocker to this PR. Looks good 👍

packages/peregrine/lib/store/reducers/app.js Show resolved Hide resolved
packages/venia-ui/lib/RootComponents/CMS/cms.js Outdated Show resolved Hide resolved
@dhaecker
Copy link
Collaborator

QA approved

@davemacaulay davemacaulay merged commit 51103c5 into magento:develop Jun 12, 2020
@m2-community-project m2-community-project bot moved this from Review in Progress to Done in Pull Request Progress Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:peregrine pkg:venia-ui version: Minor This changeset includes functionality added in a backwards compatible manner.
Development

Successfully merging this pull request may close these issues.

[bug]: Home Page query is not fetching from network/updating cache
8 participants