-
Notifications
You must be signed in to change notification settings - Fork 6
Updated README.md to have the way to handle the no fetch handler case and the empty fetch handler case. #15
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
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
To deal with WICG#14, the explainer has been updated. It started to have the section that explains how the static routing API works with no fetch handler case. Its explanation on handling the empty fetch handler has been updated.
@sisidovski @azaika Can I ask you to review the explainer update? |
sisidovski
reviewed
Dec 1, 2023
- clarified "cache" won't run the fetch handler. - said "return a promise" instead of "raise exceptions" because `addRoutes()` returns a promise. - clarified why "empty fetch handler" should not return a promise rejected with a TypeError.
azaika
reviewed
Dec 1, 2023
As suggested in the comment, I have clarified the empty fetch handler behaves different for fetch-event and race-network-and-fetch-handler cases.
sisidovski
approved these changes
Dec 4, 2023
The "cache" source should look up a request from the cache storage even if there is no fetch handler. | ||
Moreover, it does not need to run the fetch handlers regardless of cache hit or cache miss. | ||
On the other hand, the "fetch-event" and "race-network-and-fetch-handler" sources run the fetch handlers. | ||
`addRoutes()` should return a promise rejected with a *TypeError* if these sources are used while no fetch handlers are set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good clarification 👍
Add the link to the skip empty fetch handler PR.
LGTM. Thanks! |
azaika
approved these changes
Dec 4, 2023
Thank you for the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To address #14, the explainer has been updated to express the way to deal with the no fetch handler case and the empty fetch handler case.