Skip to content

[core-rest-pipeline] Remove oscpu from OS sniffing#24809

Merged
mpodwysocki merged 2 commits intomainfrom
bug/24758
Feb 9, 2023
Merged

[core-rest-pipeline] Remove oscpu from OS sniffing#24809
mpodwysocki merged 2 commits intomainfrom
bug/24758

Conversation

@mpodwysocki
Copy link
Contributor

Packages impacted by this PR

  • [@azure/core-rest-pipeline]

Issues associated with this PR

#24758

Describe the problem that is addressed by this PR

Relies on the deprecated oscpu. This uses the non-standard globalThis.navigator.userAgentData.platform and falls back to the deprecated globalThis.navigator.platform.

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

We could have removed the OS checks overall, but we can get better telemetry to keep the OS detection.

Are there test cases added in this PR? (If not, why?)

Provide a list of related PRs (if any)

Command used to generate this PR:**(Applicable only to SDK release request PRs)

Checklists

  • Added impacted package name to the issue description
  • Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
  • Added a changelog (if necessary)

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@xirzec
Copy link
Member

xirzec commented Feb 9, 2023

Could you post an example UA before/after this change from Chrome, Edge, and Firefox?

@xirzec xirzec added the Client This issue points to a problem in the data-plane of the library. label Feb 9, 2023
Copy link
Member

@witemple-msft witemple-msft left a comment

Choose a reason for hiding this comment

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

This may change the formatting of some platform specific strings (like UA we send), but they'll contain the same basic info.

Copy link
Member

@jeremymeng jeremymeng left a comment

Choose a reason for hiding this comment

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

Looks good! :shipit:

@mpodwysocki
Copy link
Contributor Author

@xirzec Note that this change will affect Blink-Based browsers with Chrome/Edge/Opera. Note that Safari and Firefox do not implement the navigator.userAgentData.platform API, so the results will not change.

Before with navigator.platform

  • Windows - win32
  • macOS - macIntel
  • Linux - linux (architecture)

After using Blink Based Browsers (Chrome/Edge/Opera/etc) with navigator.userAgentData.platform

  • Windows - windows
  • macOS - macOS
  • Linux - linux

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Azure.Core Client This issue points to a problem in the data-plane of the library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants