-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Omit service worker settings when service worker is disabled #168192
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
Conversation
|
Still need to add test to validate the new behavior |
af6d299 to
91ad8eb
Compare
mdebbar
left a comment
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.
| // in question. | ||
| final String serviceWorkerVersion = Random().nextInt(4294967296).toString(); | ||
| final String? serviceWorkerVersion = | ||
| includeServiceWorkerSettings ? Random().nextInt(1 << 32).toString() : null; |
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.
I believe 1 << 32 in JavaScript will not give the expected 4294967296 result (it computes to 1 AFAIK). Looks more readable but not quite as good. Maybe use 1 << 31, or revert to the actual value?
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.
This is tool code that's only every run on the VM. So JS behavior does not matter.
…#168192) Omit the `serviceWorkerVersion` when the feature disabled, specifically when `--pwa-strategy none`

Omit the
serviceWorkerVersionwhen the feature disabled, specifically when--pwa-strategy none