implement platform info #412
Conversation
commit: |
size-limit report 📦
|
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| : () => 'unknown', | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition, @typescript-eslint/no-deprecated | ||
| platform: typeof g.navigator?.platform === 'string' | ||
| ? () => normalizeMachine(g.navigator.platform.split(' ')[0]) // eslint-disable-line @typescript-eslint/no-deprecated |
There was a problem hiding this comment.
The platform function should call normalizeOSType() instead of normalizeMachine() since it's meant to return the OS type, not the machine architecture.
| | 'arm64' | ||
| | 'arm' | ||
| | 'i686' | ||
| | 'ia32' |
There was a problem hiding this comment.
[nitpick] The Machine type includes 'ia32' but the normalization logic maps 'ia32' to 'x32'. Consider whether 'ia32' should be in the union type if it's always normalized to 'x32'.
| | 'ia32' |
| | 's390x' | ||
| | 'x86_64') | ||
|
|
There was a problem hiding this comment.
[nitpick] The Machine type includes 'x86_64' but the normalization logic maps it to 'x64'. Consider whether 'x86_64' should be in the union type if it's always normalized to 'x64'.
| | 's390x' | |
| | 'x86_64') | |
| | 's390x') |
| memoryFree: number; | ||
| memoryTotal: number; | ||
| osType: OS; | ||
| runtime: Omit<JSRuntime, 'browser' | 'bun' | 'deno' | 'node'>; |
There was a problem hiding this comment.
This Omit excludes all known runtime values ('browser', 'bun', 'deno', 'node'), which would result in never. This makes PlatformMetricsBase unusable since the runtime field would have no valid values.
| runtime: Omit<JSRuntime, 'browser' | 'bun' | 'deno' | 'node'>; | |
| runtime: JSRuntime; |
| | 'i686' | ||
| | 'ia32' | ||
| | 'loong64' | ||
| | 'mips64' |
There was a problem hiding this comment.
[nitpick] The Machine type includes 'mips64' but there's no corresponding entry in the test file test/platform-normalize-arch.test.ts (which tests 'mips' and 'mipsel'). Consider adding test coverage for 'mips64' if it's a supported architecture.
fixes #176
It is actually quite annoying to implement this without trying to start implementing stuff like useragent parsing. So the focus is nodejs.
I am not happy with some of the names.
Still need some input on how to show this information.