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

Add page reloading in offline.php templates #697

Merged
merged 8 commits into from
Feb 24, 2022

Conversation

thelovekesh
Copy link
Collaborator

@thelovekesh thelovekesh commented Feb 14, 2022

Fixes #438

Automatically reload the offline page once the user is back online.

Tasks Done:

  • Add JS to automatically reload the offline page once the user is back online
offline-page-reload.mp4

* Automatically reload the offline page once the user is back online.
@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2022

Codecov Report

Merging #697 (9f2e7c1) into develop (6efeba2) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop     #697      +/-   ##
=============================================
- Coverage      19.05%   19.00%   -0.06%     
  Complexity       324      324              
=============================================
  Files             55       55              
  Lines           2062     2068       +6     
=============================================
  Hits             393      393              
- Misses          1669     1675       +6     
Flag Coverage Δ
php 19.00% <0.00%> (-0.06%) ⬇️
unit 19.00% <0.00%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
wp-includes/template.php 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6efeba2...9f2e7c1. Read the comment docs.

@ankitrox ankitrox removed their request for review February 14, 2022 10:48
wp-includes/template.php Outdated Show resolved Hide resolved
wp-includes/template.php Outdated Show resolved Hide resolved
wp-includes/template.php Outdated Show resolved Hide resolved
* @since 0.7
*/
function wp_service_worker_offline_page_reload() {
if ( ! is_offline() ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be useful to also do the reload in the case of server being down.

Also, something else that comes to mind: what happens if the error page is served in response to a POST request? If so, each reload will cause a re-POST message from the browser and that's probably not desired.

Suggested change
if ( ! is_offline() ) {
if ( ! is_offline() || ! is_500() || 'GET' !== $_SERVER['REQUEST_METHOD'] ) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. For sure we need to check the method of the request. I have implemented these changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As part of #728, I just realized that this check will never do anything:

if ( isset( $_SERVER['REQUEST_METHOD'] ) && 'GET' !== $_SERVER['REQUEST_METHOD'] ) {
return;
}

This is because the offline template is always fetched via a GET request by the service worker when it is installing, so it will never not be GET. We need an alternative way of preventing the code from running on responses to non-GET requests.

Contrary to what I thought above, each reload does not cause the browser re-POST message: #728 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe I've addressed this via #763

Copy link
Collaborator

Choose a reason for hiding this comment

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

See #763

wp-includes/template.php Show resolved Hide resolved
@westonruter
Copy link
Collaborator

Note that testing this is a bit tricky because the script is part of the precached template, so without a version bump it won't show up.

@westonruter
Copy link
Collaborator

Let's just add a PHPUnit test to ensure that the cases are covered for this function outputting the script.

@thelovekesh
Copy link
Collaborator Author

Note that testing this is a bit tricky because the script is part of the precached template, so without a version bump it won't show up.

@westonruter Tests added 👍

@westonruter westonruter added this to the 0.7 milestone Feb 16, 2022
@thelovekesh
Copy link
Collaborator Author

@westonruter Can you please confirm if this PR needs no more improvements?

Copy link
Collaborator

@westonruter westonruter 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 good, yes.

@westonruter westonruter merged commit 22b997f into develop Feb 24, 2022
@westonruter westonruter deleted the feature/438-reload-offline-page branch February 24, 2022 06:13
@westonruter
Copy link
Collaborator

I just realized one thing that should be done to improve this. Namely, if you try going to /?wp_error_template=offline then the result is an infinite reload as it tries to go back online. Accessing this URL directly is key for being able to see how the offline template looks during development. So I think the logic to auto-reload should be disabled if this is true:

( new URLSearchParams(location.search.substr(1)) ).has( 'wp_error_template' )

@thelovekesh
Copy link
Collaborator Author

thelovekesh commented Feb 28, 2022

then the result is an infinite reload

Thanks for pointing it out and it's a great finding. I just checked it after your comment. We should add it else it will cause infinite reloads during development.

@westonruter Should I push a commit for this?

@westonruter
Copy link
Collaborator

@thelovekesh Yes please! Thanks.

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

Successfully merging this pull request may close these issues.

Automatically reload the offline page once the user is back online
4 participants