Skip to content

Simplify getDeviceSpec and navigator test cleanup#27010

Merged
ChumpChief merged 5 commits into
microsoft:mainfrom
ChumpChief:container-runtime-navigator-cleanup
Apr 22, 2026
Merged

Simplify getDeviceSpec and navigator test cleanup#27010
ChumpChief merged 5 commits into
microsoft:mainfrom
ChumpChief:container-runtime-navigator-cleanup

Conversation

@ChumpChief
Copy link
Copy Markdown
Contributor

@ChumpChief ChumpChief commented Apr 13, 2026

Description

Now that we require Node 22 (#26934), navigator is always available (in both browsers
and Node 22). This simplifies getDeviceSpec() and its tests:

  • Remove the typeof navigator === "object" guard and try/catch from getDeviceSpec()
    since navigator is always present in supported environments.
  • Replace the as any cast for deviceMemory with a typed intersection cast
    (Navigator & { deviceMemory?: number }), eliminating three eslint disables.
  • Remove the null and undefined navigator test cases since no supported environment
    has a missing navigator.
  • Add a Node-native navigator test case that verifies hardwareConcurrency is a number
    and deviceMemory is undefined (matching Node 22's built-in navigator behavior).
  • Simplify restoreNavigator by removing the delete branch (Node 22 always provides
    a navigator descriptor to restore).

These improvements were identified during review of #26938.

AB#68707

Reviewer Guidance

The review process is outlined on this wiki page.

- Remove null/undefined navigator guards from getDeviceSpec() since
  navigator is always present in Node 22+ and browsers.
- Add comments on behavior differences between Node and browsers
  (deviceMemory is browser-only, hardwareConcurrency is available in
  both).
- Remove null/undefined navigator test cases since no supported
  environment has a missing navigator.
- Add Node-native navigator test case verifying hardwareConcurrency
  is set and deviceMemory is undefined.
- Simplify restoreNavigator by removing the delete branch (Node 22+
  always provides a navigator descriptor to restore).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ChumpChief ChumpChief marked this pull request as ready for review April 13, 2026 19:32
@ChumpChief ChumpChief requested review from Copilot and jason-ha April 13, 2026 19:32
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Simplifies getDeviceSpec() and related tests under the assumption that Node 22+ is the supported runtime and navigator is always present.

Changes:

  • Simplifies getDeviceSpec() by removing the runtime guard/try-catch and using a typed cast for deviceMemory.
  • Updates hardware stats tests to remove null/undefined navigator scenarios and add a Node-native navigator case.
  • Simplifies navigator test restoration logic by always restoring the original property descriptor.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
packages/runtime/container-runtime/src/containerRuntime.ts Removes guard/try-catch around navigator access and uses typed intersection for deviceMemory.
packages/runtime/container-runtime/src/test/hardwareStats.spec.ts Updates navigator mocking/restoration and modernizes test cases for Node 22 behavior.

Comment thread packages/runtime/container-runtime/src/containerRuntime.ts
Comment thread packages/runtime/container-runtime/src/test/hardwareStats.spec.ts Outdated
Comment thread packages/runtime/container-runtime/src/test/hardwareStats.spec.ts Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Where do we document to customers that Node 22+ is required to use FF client?
Is there any enforcement via package management?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, I forgot to update the readmes 🤦‍♂️ the main one is https://github.com/microsoft/FluidFramework/blob/main/ClientRequirements.md but the template gets pasted in every README. I've opened #27039 to update the readmes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay - lets hold this breaking change until at least 2.100.
Please do also add a breaking change changeset. This is one of those rare cases where we can make a breaking change without a major. ... If we want to do yearly majors as recently discussed, then April might be the time to do so each year and include the Node requirement.

Copy link
Copy Markdown
Contributor

@jason-ha jason-ha Apr 16, 2026

Choose a reason for hiding this comment

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

Good to proceed now that 2.100 is the active window.
Please do add changeset highlighting Node22 requirement - should be a general all packages note.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Opened #27116 for the all-packages breaking change notice and also ensuring the engines field is correct for all workspaces. For this PR, the latest iteration has an additional specific changeset detailing the impact of this navigator dependency.

Wraps the descriptor capture in an IIFE so the assert narrows the
type to a definite PropertyDescriptor, eliminating the non-null
assertion and eslint disable in restoreNavigator.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 20, 2026

🔭 PR Review Fleet Report

Note

This report is generated by an experimental AI review fleet and is provided as a beta feature. Findings are a starting point for discussion, not a gate. Use your own judgement.

Verdict: ❌ Request Changes

0 Exterminate, 1 Squash, 0 Investigate

Findings

Sev # Area File What Fix
🦟 Squash H1 Correctness packages/runtime/container-runtime/src/containerRuntime.ts:680 The removal of the try/catch and typeof navigator === "object" guard means that in any environment where navigator is not a global (Node.js < 22, server-side JS runtimes, non-browser environments that happen to use this package), accessing navigator directly will throw ReferenceError: navigator is not defined. The old code intentionally caught this case; the new code silently assumes navigator is always present. The package itself declares no engine requirement for Node >=22, so downstream consumers on older Node versions hit this crash path during ContainerRuntime.loadRuntime2 (called at line 2140, which spreads getDeviceSpec() into a telemetry send) failing container initialization entirely on every connect. — Restore the guard: wrap the body with if (typeof navigator !== "undefined" && navigator !== null), returning {} otherwise; or, if Node

View workflow run

ChumpChief and others added 2 commits April 21, 2026 11:13
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ChumpChief ChumpChief requested a review from a team as a code owner April 21, 2026 20:25
@ChumpChief ChumpChief merged commit 936562b into microsoft:main Apr 22, 2026
31 checks passed
@ChumpChief ChumpChief deleted the container-runtime-navigator-cleanup branch April 22, 2026 20:30
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.

5 participants