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

Revive of PR #1686. #1

Merged
merged 20 commits into from
Dec 6, 2023
Merged

Conversation

yoshisatoyanagisawa
Copy link
Owner

@yoshisatoyanagisawa yoshisatoyanagisawa commented Nov 28, 2023

I used to work on the specification update to support the ServiceWorker static routing API
(https://github.com/WICG/service-worker-static-routing-api) w3c#1686

However, I accidentally closed it by force-sync to the ServiceWorker specification's repository HEAD.

This CL is for reviving it.


Preview | Diff

I used to work on the specification update to support the
ServiceWorker static routing API
(https://github.com/WICG/service-worker-static-routing-api)
w3c#1686

However, I accidentally closed it by force-sync to the ServiceWorker
specification's repository HEAD.

This CL is for reviving it.
@yoshisatoyanagisawa
Copy link
Owner Author

@domenic @sisidovski Can I ask your review for this?
My preference to this PR is dropping something controversial and move forward with something we can agree.

docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
Copy link

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Only one significant issue, about what happens if the "urlPattern" dictionary member is not present.

@yoshisatoyanagisawa
Copy link
Owner Author

Only one significant issue, about what happens if the "urlPattern" dictionary member is not present.

I have updated the specification to make the case handled.
In that case, VerifyRouterRule() returns false, and GetRouterSource() ignores.

Copy link

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM with nits

docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
@yoshisatoyanagisawa
Copy link
Owner Author

@sisidovski Can I ask you to take another look? Does it look good?

docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
Copy link
Collaborator

@sisidovski sisidovski left a comment

Choose a reason for hiding this comment

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

LGTM w/ above comments.

@yoshisatoyanagisawa yoshisatoyanagisawa merged commit 745eeeb into static_routing_api Dec 6, 2023
1 check passed
@yoshisatoyanagisawa yoshisatoyanagisawa deleted the yy_workspace branch January 11, 2024 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants