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

feat: expose cpu info #6

Merged
merged 10 commits into from
May 23, 2024
Merged

feat: expose cpu info #6

merged 10 commits into from
May 23, 2024

Conversation

edvujic
Copy link
Contributor

@edvujic edvujic commented May 14, 2024

Description

This PR introduces a new feature to expose the CpuInfo private property to show the number of CPU cores available on a system.

JIRA Ticket

WEBEX-379500

Changes

  • Exposed the getCpuInfo()
  • Added unit tests to verify the correctness of the core count retrieval.

src/web-capabilities.ts Outdated Show resolved Hide resolved
@edvujic edvujic requested a review from SomeBody16 May 21, 2024 18:19
src/cpu-info.ts Outdated Show resolved Hide resolved
src/web-capabilities.ts Outdated Show resolved Hide resolved
src/cpu-info.spec.ts Outdated Show resolved Hide resolved
src/web-capabilities.spec.ts Outdated Show resolved Hide resolved
@edvujic edvujic requested a review from brycetham May 21, 2024 21:18
src/cpu-info.ts Outdated Show resolved Hide resolved
src/cpu-info.spec.ts Outdated Show resolved Hide resolved
@edvujic edvujic requested a review from antsukanova May 22, 2024 13:30
antsukanova
antsukanova previously approved these changes May 22, 2024
src/cpu-info.ts Outdated
*
* @returns The number of logical CPU cores, or undefined if not available.
*/
static getNumLogicalCores(): number {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed that the JSDoc says this method could return "undefined if not available" but here we say it always returns a number. Checking the MDN docs it does seem like all major browsers support it, though I suppose it is possible that someone might use an unsupported browser.

Should we set the return type to number | undefined here?

Copy link
Contributor Author

@edvujic edvujic May 22, 2024

Choose a reason for hiding this comment

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

Yes, that was my initial idea as well. I checked the MDN docs, but when we return number | undefined TS is giving us an error for:

Screenshot 2024-05-22 at 11 04 52 AM

We could avoid this by using !, but we've already agreed to not use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option could be using something like:

static isCapableOfSending1080pVideo(): CapabilityState {
    const numCores = CpuInfo.getNumLogicalCores();
    if (numCores === undefined) {
      return CapabilityState.UNKNOWN;
    }
    if (numCores < 2) {
      return CapabilityState.NOT_CAPABLE;
    }
    return CapabilityState.CAPABLE;
  }

src/cpu-info.spec.ts Outdated Show resolved Hide resolved
src/cpu-info.spec.ts Outdated Show resolved Hide resolved
@edvujic edvujic requested a review from brycetham May 22, 2024 18:21
Copy link
Collaborator

@brycetham brycetham 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, thanks for making the changes. I think once we add more methods to CpuInfo it will become even more obvious the benefits of making it its own class.

One last thing before I approve: could we update the README with an example using the new CpuInfo API (similar to BrowserInfo)?

@edvujic edvujic requested a review from brycetham May 22, 2024 20:12
@edvujic edvujic merged commit 6e43d38 into main May 23, 2024
1 check passed
github-actions bot pushed a commit that referenced this pull request May 23, 2024
# [1.3.0](v1.2.0...v1.3.0) (2024-05-23)

### Features

* expose cpu info ([#6](#6)) ([6e43d38](6e43d38))
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.

4 participants