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

Introduce Client Hints API #864

Merged
merged 22 commits into from
Jun 6, 2023
Merged

Introduce Client Hints API #864

merged 22 commits into from
Jun 6, 2023

Conversation

danieljackins
Copy link
Contributor

@danieljackins danieljackins commented May 20, 2023

This PR introduces the Client Hints API. We will always request the low entropy hints, but each high entropy hint will only be requested as set in the options.

For example,

AnalyticsBrowser.load( ..., { highEntropyValues: ['bitness', 'architecture', 'model', 'platformVersion', 'fullVersionList'] } )

Would populate context.userAgentData with something like

{ "architecture": "x86", "bitness": "64", "brands": [ { "brand": "Google Chrome", "version": "113" }, { "brand": "Chromium", "version": "113" }, { "brand": "Not-A.Brand", "version": "24" } ], "fullVersionList": [ { "brand": "Google Chrome", "version": "113.0.5672.92" }, { "brand": "Chromium", "version": "113.0.5672.92" }, { "brand": "Not-A.Brand", "version": "24.0.0.0" } ], "mobile": false, "model": "", "platform": "macOS", "platformVersion": "13.1.0" }

[] I've included a changeset (psst. run yarn changeset. Read about changesets here).

@changeset-bot
Copy link

changeset-bot bot commented May 20, 2023

🦋 Changeset detected

Latest commit: 0df5553

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@segment/analytics-next Minor
@segment/analytics-core Minor
@segment/analytics-node Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@danieljackins danieljackins requested review from silesky, chrisradek and a team May 20, 2023 01:42
@danieljackins danieljackins marked this pull request as ready for review May 25, 2023 00:22
@danieljackins danieljackins requested a review from silesky May 25, 2023 00:22

/**
* Array of high entropy Client Hints to request. These may be rejected by the user agent - only required hints should be requested.
* @return {Object} Object containing low entropy hints and successfully resolved high entropy hints
Copy link
Contributor

@silesky silesky Jun 1, 2023

Choose a reason for hiding this comment

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

These jsdoc comments seem out of place :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh you mentioned wanting a doc string and I thought you meant for this interface 😅 which did you mean?

Copy link
Contributor

@silesky silesky Jun 1, 2023

Choose a reason for hiding this comment

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

Oh, I see what you did. The docstring goes in the InitOptions interface, above the specific setting.


// https://wicg.github.io/ua-client-hints/#navigatoruadata
export interface NavigatorUAData extends UALowEntropyJSON {
getHighEntropyValues(hints: string[]): Promise<UADataValues>
Copy link
Contributor

@silesky silesky Jun 1, 2023

Choose a reason for hiding this comment

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

Should we use HighEntropyHint type here?

@silesky
Copy link
Contributor

silesky commented Jun 5, 2023

Just remembered, we should document this new userAgentData object like we do our other reserved properties in the public docs.

@@ -84,6 +85,10 @@ export function segmentio(
? batch(apiHost, deliveryStrategy.config)
: standard(deliveryStrategy?.config as StandardDispatcherConfig)

const userAgentData = await clientHints(
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on wrapping this in a try/catch?

You've got good error handling in clientHints already - my paranoia stems from the whole client hints API still being marked experimental, and safari/firefox haven't implemented it yet. My fear is they expose a partial implementation without toJSON (I think super unlikely scenario but a failure would mean the segment plugin fails to load which is pretty critical)

@danieljackins danieljackins merged commit 6cba535 into master Jun 6, 2023
@danieljackins danieljackins deleted the client-hints branch June 6, 2023 20:34
@github-actions github-actions bot mentioned this pull request Jun 6, 2023
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