feat: add navigator.platform #30795
feat: add navigator.platform #30795
Conversation
|
I think it needs to be consistent with deno/cli/tsc/dts/lib.deno.ns.d.ts Lines 4470 to 4479 in 7938d5d |
|
But the spec names them differently (what node does) https://developer.mozilla.org/en-US/docs/Web/API/Navigator/platform#value |
|
Though I guess the spec says "for example" so maybe its ok with Deno values ? |
|
Yeah, it's better to use values that match other runtimes. Line 107 in 7938d5d https://github.com/denoland/deno_core/blob/2770aa8f18800b0a4a997954e9482205764fea97/core/00_infra.js#L58-L78 |
iuioiua
left a comment
iuioiua
left a comment
There was a problem hiding this comment.
Do types need to be adjusted somewhere too?
| * @param {string} platform | ||
| * @returns {string} | ||
| */ | ||
| function getNavigatorPlatform(arch, platform) { |
There was a problem hiding this comment.
Maybe use a switch statement?
|
I updated the code to use core.build I'm not sure what other changes are needed |
I'll make sure this is landed before 2.6. |
|
@sigmaSd could you please rebase this one so it can get landed? |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for the navigator.platform property to Deno's runtime, addressing issue #24117. The implementation follows browser conventions by returning platform-specific strings that match modern browser behavior.
- Implements
getNavigatorPlatform()function that maps Deno's OS/architecture to browser-compatible platform strings - Adds
navigator.platformproperty getter with proper Web IDL branding - Includes the new property in Navigator's custom inspect output
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| default: | ||
| return `${StringPrototypeToUpperCase(platform[0])}${ | ||
| StringPrototypeSlice(platform, 1) |
There was a problem hiding this comment.
StringPrototypeSlice is used on line 57 but is not imported from primordials. Add it to the destructuring assignment on line 13:
const {
ObjectDefineProperties,
ObjectPrototypeIsPrototypeOf,
SymbolFor,
StringPrototypeToUpperCase,
StringPrototypeSlice,
} = primordials;There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
I just pressed the implement change button thought it will just change the code
I'll update this
| default: | ||
| return `${StringPrototypeToUpperCase(platform[0])}${ | ||
| StringPrototypeSlice(platform, 1) | ||
| } ${arch}`; |
There was a problem hiding this comment.
The default case doesn't check if platform is empty before accessing platform[0], which could cause an error. Additionally, openbsd and android are valid OS values in Deno (found in repo/ext/fs/30_fs.js) but are not explicitly handled and would fall through to the default case. Consider adding explicit cases for these OSes:
case "openbsd":
return `OpenBSD ${arch}`;
case "android":
return `Linux ${arch}`; // Android typically reports as Linux| default: | |
| return `${StringPrototypeToUpperCase(platform[0])}${ | |
| StringPrototypeSlice(platform, 1) | |
| } ${arch}`; | |
| case "openbsd": | |
| return `OpenBSD ${arch}`; | |
| case "android": | |
| return `Linux ${arch}`; // Android typically reports as Linux | |
| default: | |
| if (typeof platform === "string" && platform.length > 0) { | |
| return `${StringPrototypeToUpperCase(platform[0])}${ | |
| StringPrototypeSlice(platform, 1) | |
| } ${arch}`; | |
| } else { | |
| return arch || "Unknown"; | |
| } |
There was a problem hiding this comment.
Seems logical but I just copy pasted the node implementation, should I apply this?
There was a problem hiding this comment.
no, platform is never an empty string. if it is, we dont support that system anyways
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
runtime/js/98_global_scope_window.js (1)
25-61:getNavigatorPlatformlogic and memoization look sound; consider clearer namingThe platform computation mirrors established runtime behavior for major OSes (
darwin→MacIntel,windows→Win32, etc.) and usesmemoizeLazyconsistently withuserAgentandlanguage. Usingcore.build.archandcore.build.osis in line with the earlier review guidance to avoid importing Node internals, and the function always returns a string, so caching with anullsentinel is safe.As a minor readability tweak, both the parameter
platformingetNavigatorPlatform(arch, platform)and the top-levelconst platform = memoizeLazy(...)are named identically, which can be a bit confusing when scanning the file. Renaming one of them (e.g.,os/osNamein the helper, ornavigatorPlatformfor the memoized value) would make it easier to distinguish between the OS identifier and the computednavigator.platformstring.-function getNavigatorPlatform(arch, platform) { - switch (platform) { +function getNavigatorPlatform(arch, osName) { + switch (osName) { @@ - default: - return `${StringPrototypeToUpperCase(platform[0])}${ - StringPrototypeSlice(platform, 1) - } ${arch}`; + default: + return `${StringPrototypeToUpperCase(osName[0])}${ + StringPrototypeSlice(osName, 1) + } ${arch}`; } } @@ -const platform = memoizeLazy(() => - getNavigatorPlatform(core.build.arch, core.build.os) -); +const navigatorPlatform = memoizeLazy(() => + getNavigatorPlatform(core.build.arch, core.build.os) +);(plus updating the getter below to call
navigatorPlatform()instead ofplatform()).Also applies to: 101-103
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
runtime/js/98_global_scope_window.js(5 hunks)
🔇 Additional comments (3)
runtime/js/98_global_scope_window.js (3)
13-15: Primordials additions correctly cover new string operationsImporting
StringPrototypeToUpperCaseandStringPrototypeSlicefromprimordialsmatches how other helpers in this file are sourced and resolves the prior missing-import concern cleanly. No issues from a correctness or style perspective.
78-78: Including"platform"in the custom inspect keys is consistentAdding
"platform"to the filtered inspect proxy keys keepsnavigator’s developer-facing representation in sync with the new property, matching howhardwareConcurrency,userAgent, andlanguageare exposed.
153-161:Navigator.platformgetter wiring matches existing patterns; ensure type defs alignThe new
platformdescriptor is consistent with the other navigator properties: branded viawebidl.assertBranded, enumerable, configurable, and backed by the memoized function. This should integrate cleanly with the rest of the runtime initialization.One follow-up to double-check outside this file: make sure the corresponding TypeScript/lib definitions for
Navigator(e.g., inlib.deno.ns.d.tsor related Web IDL bindings) now includeplatform: string, so users get proper typing and cross-runtime parity.
|
It seems like I need to update a test , but its not clear to me where exactly I need to make the change |
|
Errors raised here, make no sense. I think cache is broken or this PR is getting assigned to a specific runner |
@sigmaSd you need to specify it in |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/globals_test.ts (1)
236-252: navigatorPlatformExists test is good; consider asserting on other OSes tooThe OS-specific expectations for Linux, macOS, and Windows look correct and align with the new
navigator.platformbehavior. One small improvement: on otherDeno.build.osvalues this test currently does nothing, so regressions there would go unnoticed. You could add adefaultbranch that at least assertstypeof navigator.platform === "string"(or similar) to keep the test meaningful if more platforms are supported later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/node_compat/config.toml(1 hunks)tests/unit/globals_test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
tests/unit/globals_test.ts
🧬 Code graph analysis (1)
tests/unit/globals_test.ts (1)
runtime/js/98_global_scope_window.js (1)
navigator(86-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug macos-x86_64
- GitHub Check: lint debug linux-x86_64
- GitHub Check: build libs
🔇 Additional comments (1)
tests/node_compat/config.toml (1)
770-770: Node-compat navigator test correctly enabledAdding
"parallel/test-navigator.js" = {}here is the right way to wire the Node navigator test into the compat suite; placement and config look consistent with surrounding entries.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/globals_test.ts (1)
237-255: Handle futureDeno.build.osvalues explicitly in the testThe test covers Linux/macOS/Windows, but if
Deno.build.osever gains another value, this test will silently do nothing. Adding adefaultbranch will make sure new platforms are intentionally handled.Deno.test(function navigatorPlatformExists() { switch (Deno.build.os) { @@ case "windows": { // deno-lint-ignore no-undef assertEquals(navigator.platform, "Win32"); break; } + default: { + throw new Error(`Unhandled Deno.build.os: ${Deno.build.os}`); + } } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/globals_test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
tests/unit/globals_test.ts
🧬 Code graph analysis (1)
tests/unit/globals_test.ts (1)
runtime/js/98_global_scope_window.js (1)
navigator(86-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug macos-x86_64
- GitHub Check: lint debug linux-x86_64
|
I added platform type to lib.dom but it still error in type checking saying it doesn't exist |
5 participants
Fix #24117
This seems not like the correct approach ( importing node), but its the easiest way and I thought maybe its a good start for instructions on how to add this
I just copied https://github.com/nodejs/node/blob/main/lib/internal/navigator.js#L41
Also there seems to be a test here https://github.com/denoland/node_test/blob/main/test/parallel/test-navigator.js#L35 how do I activate it ?