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

DocumentHead: Only reset unreadCount on ROUTE_SET #25501

Merged
merged 1 commit into from
Jun 15, 2018

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Jun 14, 2018

Variation of #25005 (without requiring #24980).

This is the conclusion of a call with @a8dar and @sirreal, based on previous work and investigation found in #25493 and #23961, etc.

Reverts most of #8224, except for its main objective: to reset unreadCount upon route change. However, resetting all the other parts of document-head (title, meta, and links) turned out to be undesirable found documented in the issues and PRs linked from this PR :)

Fixes #24977. Also fixes #23924.

Stealing @a8dar's testing instructions from #25493:

  1. Checkout the branch and start the server npm start. You can also do it on calypso.live, but locally you have more control on the cache.
  2. Access https://calypso.localhost:3000/log-in
  3. See the page title. Should contain 'Log In'. In production probably it doesn't.
  4. Look at the page source (not Inspector, but rather real page source that was initially generated). You should see a rel=canonical pointing to the /log-in source.
  5. [Skipping step 5 from Canonical URLs: fix /log-in DocumentHead: canonical, title, and meta #25493 which doesn't apply]
  6. Access themes section and make sure everything is ok, as in production. Themes section is also SSR-ed and uses the cache. Watch for the page title, hoping that everything works as expected (or better than expected).

Now reload any of these pages, and verify that the site title is still intact, and that the page source continues to include canonical links.


This finally fixes server-side caching of document-head stuff.
Potential (but less crucial) follow-ups include #24980 and #25003.

@matticbot
Copy link
Contributor

@ockham ockham requested review from sirreal, aduth, Copons and a user June 14, 2018 17:21
@ockham ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jun 14, 2018
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Tested and looks good, works as expected. Both title and canonical seem to be now consistent on SSR.

I also looked for sections where the title would not be reset when switching the route and found the 'Domains' one (practically when entering the Domains from the left side menu, the old title would persist). We could fix that later.

WARNING: one test seems to have failed, better to check what's happening before merging.

@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 15, 2018
@sirreal
Copy link
Member

sirreal commented Jun 15, 2018

All tests passing now 💚

@ockham ockham merged commit 04fd4b7 into master Jun 15, 2018
@ockham ockham deleted the update/dont-reset-document-head-on-route-set branch June 15, 2018 10:31
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 15, 2018
@dmsnell dmsnell added Framework [Type] Bug When a feature is broken and / or not performing as intended labels Jun 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Server Side Rendering [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DocumentHead: Fix DocumentHead: reseting title on tab view changes
4 participants