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

feat(service-worker): add the option to prefer network for navigation… #38565

Conversation

klemenoslaj
Copy link
Contributor

@klemenoslaj klemenoslaj commented Aug 24, 2020

… requests

This commit introduces a new option for the service worker, called
navigationRequestStrategy, which adds the possibility to force the service worker
to always create a network request for navigation requests.
This enables the server redirects while retaining the offline behavior.

Fixes #38194

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #38194, #36099

What is the new behavior?

After enabling the freashnessForNavigationUrls flag, the navigation requests will always go through the network.
This will enable the HTTP redirects.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@sonukapoor sonukapoor added the area: service-worker Issues related to the @angular/service-worker package label Aug 25, 2020
@ngbot ngbot bot added this to the needsTriage milestone Aug 25, 2020
@klemenoslaj klemenoslaj force-pushed the service-worker-navigate-request-freshness branch from 6d061f5 to bfa9d7e Compare August 25, 2020 23:24
@klemenoslaj klemenoslaj marked this pull request as ready for review August 26, 2020 10:14
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

This looks great, @klemenoslaj 🎉
I left some minor comments, but otherwise lgtm. Impressive work (esp. for a first-time contribution) 🙇

packages/service-worker/config/src/in.ts Outdated Show resolved Hide resolved
packages/service-worker/worker/src/app-version.ts Outdated Show resolved Hide resolved
aio/content/guide/service-worker-config.md Outdated Show resolved Hide resolved
packages/service-worker/config/schema.json Outdated Show resolved Hide resolved
packages/service-worker/worker/test/happy_spec.ts Outdated Show resolved Hide resolved
packages/service-worker/worker/test/happy_spec.ts Outdated Show resolved Hide resolved
packages/service-worker/worker/test/happy_spec.ts Outdated Show resolved Hide resolved
packages/service-worker/worker/test/happy_spec.ts Outdated Show resolved Hide resolved
packages/service-worker/worker/test/happy_spec.ts Outdated Show resolved Hide resolved
packages/service-worker/worker/test/happy_spec.ts Outdated Show resolved Hide resolved
@sonukapoor sonukapoor added the feature Issue that requests a new feature label Aug 27, 2020
@klemenoslaj klemenoslaj force-pushed the service-worker-navigate-request-freshness branch 3 times, most recently from 8cbc687 to 63b2763 Compare August 28, 2020 07:44
@klemenoslaj klemenoslaj force-pushed the service-worker-navigate-request-freshness branch from 63b2763 to ac19cce Compare August 28, 2020 08:12
@klemenoslaj klemenoslaj force-pushed the service-worker-navigate-request-freshness branch from ac19cce to 0e3a0c0 Compare August 28, 2020 11:26
Copy link
Contributor Author

@klemenoslaj klemenoslaj left a comment

Choose a reason for hiding this comment

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

@gkalpak I made the suggested changes. Please have a look.

packages/service-worker/config/src/in.ts Show resolved Hide resolved
@klemenoslaj klemenoslaj force-pushed the service-worker-navigate-request-freshness branch 3 times, most recently from 47ebeca to 015fbf8 Compare August 28, 2020 14:00
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Nice, this is pretty close to ready 🎉
Here's another round of review (just minor comments) 😃

packages/service-worker/worker/test/happy_spec.ts Outdated Show resolved Hide resolved
packages/service-worker/config/schema.json Outdated Show resolved Hide resolved
packages/service-worker/worker/src/manifest.ts Outdated Show resolved Hide resolved
packages/service-worker/worker/src/app-version.ts Outdated Show resolved Hide resolved
packages/service-worker/worker/src/app-version.ts Outdated Show resolved Hide resolved
packages/service-worker/worker/test/happy_spec.ts Outdated Show resolved Hide resolved
packages/service-worker/worker/test/happy_spec.ts Outdated Show resolved Hide resolved
aio/content/guide/service-worker-config.md Outdated Show resolved Hide resolved
aio/content/guide/service-worker-config.md Outdated Show resolved Hide resolved
aio/content/guide/service-worker-config.md Outdated Show resolved Hide resolved
@klemenoslaj klemenoslaj force-pushed the service-worker-navigate-request-freshness branch 2 times, most recently from 54e9347 to 51d3afc Compare September 3, 2020 13:38
@klemenoslaj klemenoslaj force-pushed the service-worker-navigate-request-freshness branch 2 times, most recently from 9281001 to 60c547c Compare September 4, 2020 09:52
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Thx for making the changes, @klemenoslaj 👍
The code looks great. Just some more comments regarding docs 📖

packages/service-worker/worker/src/app-version.ts Outdated Show resolved Hide resolved
aio/content/guide/service-worker-config.md Outdated Show resolved Hide resolved
aio/content/guide/service-worker-config.md Show resolved Hide resolved
aio/content/guide/service-worker-config.md Outdated Show resolved Hide resolved
aio/content/guide/service-worker-config.md Show resolved Hide resolved
aio/content/guide/service-worker-config.md Outdated Show resolved Hide resolved
aio/content/guide/service-worker-config.md Outdated Show resolved Hide resolved
@klemenoslaj klemenoslaj force-pushed the service-worker-navigate-request-freshness branch from 60c547c to 23f6bda Compare September 6, 2020 20:34
@gkalpak gkalpak added the action: review The PR is still awaiting reviews from at least one requested reviewer label Sep 7, 2020
@klemenoslaj klemenoslaj force-pushed the service-worker-navigate-request-freshness branch 2 times, most recently from a52c5b3 to 21009e8 Compare September 18, 2020 07:28
@mary-poppins
Copy link

You can preview 21009e8 at https://pr38565-21009e8.ngbuilds.io/.

Copy link
Contributor

@kapunahelewong kapunahelewong left a comment

Choose a reason for hiding this comment

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

Never mind! I didn't realize that @aikidave had already reviewed! I had it up partially completed yesterday. Deleting my comments!

@kapunahelewong kapunahelewong requested review from kapunahelewong and removed request for kapunahelewong September 18, 2020 12:50
@gkalpak gkalpak dismissed kapunahelewong’s stale review September 19, 2020 09:40

Review comments deleted by reviewer.

@gkalpak gkalpak removed the request for review from kapunahelewong September 19, 2020 09:40
aio/content/guide/service-worker-config.md Outdated Show resolved Hide resolved
aio/content/guide/service-worker-config.md Outdated Show resolved Hide resolved
aio/content/guide/service-worker-config.md Outdated Show resolved Hide resolved
@gkalpak gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Sep 19, 2020
@klemenoslaj klemenoslaj force-pushed the service-worker-navigate-request-freshness branch from 21009e8 to f84d919 Compare September 19, 2020 13:54
@mary-poppins
Copy link

You can preview f84d919 at https://pr38565-f84d919.ngbuilds.io/.

@gkalpak gkalpak added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Sep 21, 2020
Copy link
Contributor

@aikithoughts aikithoughts left a comment

Choose a reason for hiding this comment

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

A minor comment, but nothing blocking. Otherwise, LGTM.

Reviewed-for: required-docs-approvers

aio/content/guide/service-worker-config.md Outdated Show resolved Hide resolved
… requests

This commit introduces a new option for the service worker, called
`navigationRequestStrategy`, which adds the possibility to force the service worker
to always create a network request for navigation requests.
This enables the server redirects while retaining the offline behavior.

Fixes angular#38194
@klemenoslaj klemenoslaj force-pushed the service-worker-navigate-request-freshness branch from f84d919 to 2025600 Compare September 22, 2020 06:25
@mary-poppins
Copy link

You can preview 2025600 at https://pr38565-2025600.ngbuilds.io/.

@gkalpak gkalpak added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 22, 2020
@mhevery mhevery closed this in a206852 Sep 22, 2020
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker aio: preview area: service-worker Issues related to the @angular/service-worker package cla: yes feature Issue that requests a new feature target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Angular Service worker incorrectly intercepting http redirects (302)