Open
Conversation
This was referenced Apr 2, 2022
|
Any plans on getting this merged? |
randompch
reviewed
Jul 16, 2025
| } | ||
| ``` | ||
|
|
||
| `clientHints.enabled` enables client hints feature.(default by false) |
Contributor
There was a problem hiding this comment.
Suggested change
| `clientHints.enabled` enables client hints feature.(default by false) | |
| `clientHints.enabled` enables client hints feature (false by default). |
😄
|
|
||
| ## User-Agent Client Hints Support | ||
|
|
||
| To enable Client Hints, set clientHints.enabled options to true. |
Contributor
There was a problem hiding this comment.
Suggested change
| To enable Client Hints, set clientHints.enabled options to true. | |
| To enable Client Hints, set `clientHints.enabled` options to `true`. |
Comment on lines
+172
to
+179
| function deleteUndefinedProperties(obj) { | ||
| for (const key of Object.keys(obj)) { | ||
| if (typeof obj[key] === 'undefined') { | ||
| delete obj[key] | ||
| } | ||
| } | ||
| return obj | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| function deleteUndefinedProperties(obj) { | |
| for (const key of Object.keys(obj)) { | |
| if (typeof obj[key] === 'undefined') { | |
| delete obj[key] | |
| } | |
| } | |
| return obj | |
| } | |
| function deleteUndefinedProperties(obj) { | |
| return Object.fromEntries( | |
| Object.entries(obj).filter(([_, value]) => typeof value !== 'undefined') | |
| ) | |
| } |
💅
Comment on lines
+159
to
+169
| const ios = undefined | ||
| const android = undefined | ||
| const windows = platform === 'Windows' | ||
| const macOS = platform === 'macOS' | ||
| const isSafari = undefined | ||
| const isFirefox = undefined | ||
| const isEdge = hasBrand('Microsoft Edge') | ||
| const isChrome = hasBrand('Google Chrome') | ||
| const isSamsung = undefined | ||
| const isCrawler = undefined | ||
| return deleteUndefinedProperties({ mobile, mobileOrTablet, ios, android, windows, macOS, isSafari, isFirefox, isEdge, isChrome, isSamsung, isCrawler }) |
Contributor
There was a problem hiding this comment.
Why setting these to undefined if we're deleting them right away ?
Comment on lines
+144
to
+148
| const brands = uaHeader.split(',').map(b => b.trim()).map(brandStr => { | ||
| const parsed = brandStr.match(REGEX_CLIENT_HINT_BRAND) | ||
| console.log(brandStr, parsed) | ||
| return {brand: parsed[1], version: parsed[2]} | ||
| }) |
Contributor
There was a problem hiding this comment.
Suggested change
| const brands = uaHeader.split(',').map(b => b.trim()).map(brandStr => { | |
| const parsed = brandStr.match(REGEX_CLIENT_HINT_BRAND) | |
| console.log(brandStr, parsed) | |
| return {brand: parsed[1], version: parsed[2]} | |
| }) | |
| const brands = uaHeader.split(',').map(brandStr => { | |
| const [,brand,version] = brandStr.trim().match(REGEX_CLIENT_HINT_BRAND) | |
| return { brand, version } | |
| }) |
- Removed
console.log() - Removed
map()dedicated to trimming - Destructured parsed array
this part isn't defensive enough imo
Comment on lines
+150
to
+160
| results from `navigator.userAgent` are overridden. | ||
|
|
||
| ### Server Side | ||
|
|
||
| the following request headers are referred to detect a device and a platform. | ||
|
|
||
| - sec-ch-ua | ||
| - sec-ch-mobile | ||
| - sec-ch-platform | ||
|
|
||
| results from user-agent header are overridden. |
Contributor
There was a problem hiding this comment.
Suggested change
| results from `navigator.userAgent` are overridden. | |
| ### Server Side | |
| the following request headers are referred to detect a device and a platform. | |
| - sec-ch-ua | |
| - sec-ch-mobile | |
| - sec-ch-platform | |
| results from user-agent header are overridden. | |
| Results from `navigator.userAgent` are overridden. | |
| ### Server Side | |
| The following request headers are referred to detect a device and a platform. | |
| - `sec-ch-ua` | |
| - `sec-ch-mobile` | |
| - `sec-ch-platform` | |
| Results from user-agent header are overridden. |
| const macOS = platform === 'macOS' | ||
| const isSafari = undefined | ||
| const isFirefox = undefined | ||
| const isEdge = hasBrand('Microsoft Edge') |
Contributor
There was a problem hiding this comment.
these brands' list would benefit from being stored in a kind of enum
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Uh oh!
There was an error while loading. Please reload this page.