Conversation
- Coerce 'NaN' string to Number.NaN for numeric types. - Standard 'number' keyword rejects coerced NaN. - Added tests for 'NaN' string coercion. - Updated coercion logic to include NaN.
- Added "NaN" support for number coercion - Improved central coercion logic - Updated tests for new coercion behavior
|
| Name | Type |
|---|---|
| @repo/keywords | Minor |
| @repo/scope | Patch |
| @repo/types | Patch |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes add support for coercing the string "NaN" to the JavaScript NaN value. NaN guards are removed from coercion logic to allow NaN propagation, while a specific check in the number parsing morph detects the string "NaN" and returns Number.NaN. Tests validate both successful coercion and rejection by non-NaN numeric types. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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 |
github-actions
bot
added
docs
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (3)
packages/internal/keywords/src/index.ts (1)
12-18: Optimize redundant string trimming.The string is trimmed twice: once at line 14 and again at line 15. Consider storing the trimmed value to avoid redundant operations.
🔎 Proposed optimization
export const maybeParsedNumber = type("unknown").pipe((s) => { if (typeof s === "number") return s; - if (typeof s !== "string" || s.trim() === "") return s; - if (s.trim() === "NaN") return Number.NaN; - const n = Number(s); + if (typeof s !== "string") return s; + const trimmed = s.trim(); + if (trimmed === "") return s; + if (trimmed === "NaN") return Number.NaN; + const n = Number(trimmed); return Number.isNaN(n) ? s : n; });
Verify case-sensitivity requirements for "NaN" string.
The implementation only recognizes the exact string "NaN" (case-sensitive). While this matches JavaScript conventions, consider whether case-insensitive matching (e.g., "nan", "NAN") should be supported for better user experience.
packages/arkenv/src/utils/coerce.ts (1)
174-182: Update comment to reflect NaN handling change.The logic correctly propagates NaN values, but the comment at line 176 is now outdated. It states "is NaN/invalid, try boolean", but NaN is now accepted as a valid number type.
🔎 Suggested comment update
} else { const asNumber = maybeParsedNumber(original); - // If numeric parsing didn't change type (still string) or is NaN/invalid, try boolean + // If numeric parsing didn't produce a number, try boolean coercion if (typeof asNumber === "number") { record[lastKey] = asNumber; } else { record[lastKey] = maybeParsedBoolean(original); } }packages/arkenv/src/utils/coerce.test.ts (1)
195-203: Consider type narrowing instead of @ts-expect-error.The test correctly verifies NaN coercion, but the
@ts-expect-errordirective suppresses a legitimate type safety concern. Consider adding explicit type narrowing for better type safety.🔎 Suggested type narrowing approach
it("should coerce 'NaN' string to NaN number", () => { const schema = type({ VAL: "number.NaN", }); const coercedSchema = coerce(schema); const result = coercedSchema({ VAL: "NaN" }); - // @ts-expect-error + expect(result).not.toBeInstanceOf(ArkErrors); + if (result instanceof ArkErrors) return; expect(result.VAL).toBeNaN(); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/humble-lizards-judge.mdpackages/arkenv/src/utils/coerce.test.tspackages/arkenv/src/utils/coerce.tspackages/internal/keywords/src/index.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx}: Prefertypeoverinterfacefor type definitions in TypeScript
Use TypeScript 5.1+ features when appropriate
Leverageconsttype parameters for better inference in TypeScript
Use JSDoc comments for public APIs
Use tabs for indentation (configured in Biome)
Use double quotes for strings (configured in Biome)
Organize imports automatically (Biome handles this)
Avoid explicit types when TypeScript can infer them (noInferrableTypeserror)
Useas constwhere appropriate for immutable values (useAsConstAssertionerror)
Don't reassign function parameters (noParameterAssignerror)
Place default parameters last in function signatures (useDefaultParameterLasterror)
Always initialize enum values (useEnumInitializerserror)
Declare one variable per statement (useSingleVarDeclaratorerror)
Avoid unnecessary template literals (noUnusedTemplateLiteralerror)
PreferNumber.parseIntover globalparseInt(useNumberNamespaceerror)
Use kebab-case for TypeScript filenames (e.g.,create-env.ts)
Use camelCase for function names (e.g.,createEnv)
Use PascalCase for type names (e.g.,ArkEnvError)
Use UPPER_SNAKE_CASE for environment variables and constants
Include examples in JSDoc comments when helpful for public APIs
Document complex type logic with JSDoc comments
UseArkEnvErrorfor environment variable validation errors
Provide clear, actionable error messages that include the variable name and expected type
**/*.{ts,tsx}: UsecreateEnv(schema)as the main function for validated environment objects, available as the default export
Use built-in validators (host, port, url, email) fromsrc/types.tswhen available instead of custom validation
UseArkEnvErrorfor environment variable errors, not generic errors
Environment schema definitions should use built-in validators, ArkType string literals, and support default values in the schema pattern
Use logical grouping for related environment variables in schemas
Use descriptive env...
Files:
packages/internal/keywords/src/index.tspackages/arkenv/src/utils/coerce.tspackages/arkenv/src/utils/coerce.test.ts
**/index.ts
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
Use barrel exports (
index.ts) for package entry points
Files:
packages/internal/keywords/src/index.ts
**/*.{ts,tsx,json,md}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Biome for linting and formatting instead of ESLint and Prettier
Files:
packages/internal/keywords/src/index.tspackages/arkenv/src/utils/coerce.tspackages/arkenv/src/utils/coerce.test.ts
packages/arkenv/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/arktype.mdc)
packages/arkenv/**/*.ts: Use ArkType'stype()function to define schemas in environment variable definitions
Leverage ArkType's type inference for TypeScript types instead of manual type definitions
Use the scoped$type system for custom types defined inscope.ts
Keep environment variable schemas readable and TypeScript-like using ArkType syntax
Use union types for enums in ArkType schemas (e.g.,"'dev' | 'prod'") instead of separate enum definitions
Leverage ArkType's built-in types (e.g.,string.host,number.port) where possible in environment schemas
Convert ArkType validation errors toArkEnvErrorfor user-friendly error messages that include variable name and expected type
Files:
packages/arkenv/src/utils/coerce.tspackages/arkenv/src/utils/coerce.test.ts
packages/arkenv/src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Main library implementation should be in
src/create-env.ts, built-in validators insrc/types.ts, error handling insrc/errors.ts, and utilities insrc/utils.ts
Files:
packages/arkenv/src/utils/coerce.tspackages/arkenv/src/utils/coerce.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
Co-locate tests with components:
Component.tsxnext toComponent.test.tsx
**/*.test.{ts,tsx}: Test individual functions, components, and hooks in isolation with mocked dependencies using the*.test.tsor*.test.tsxsuffix
Mockprocess.envin tests for different scenarios and save/restore original env inbeforeEach/afterEachhooks
Use Vitest'sdescribe/itstructure in test files
Test edge cases including invalid and missing environment variable values
Use Vitest for testing framework
**/*.test.{ts,tsx}: Use Vitest for unit and integration tests
Test individual functions, components, and hooks in isolation with mocked dependencies in unit tests
Unit tests should focus on individual function logic and edge cases, component rendering and props, error handling and validation, and type checking
Unit tests should execute in less than 100ms per test
Mock external dependencies (clipboard, network, etc.) in unit tests
Co-locate unit test files with source files using naming convention: source file → test file (e.g., create-env.ts → create-env.test.ts)
Test component behavior, not aesthetics, and focus on what users can do and what the component guarantees through its API
Test component public API (props, events, and component contract), user behavior (clicks, typing, focus, keyboard, ARIA), state transitions, accessibility, and side effects in component tests
Do not test pure styling or CSS classes, library internals (Radix/shadcn), implementation details (hooks, setState, private variables), or visual variants in component tests
Use Testing Library with user-event for real user simulation in component tests
Query by role, name, label, and text (accessibility first) in component tests
Use beforeEach/afterEach for cleanup, not beforeAll/afterAll when possible
Keep tests fast, deterministic, and parallelizable
Mock at component boundaries (network, time, context)
Files:
packages/arkenv/src/utils/coerce.test.ts
🧠 Learnings (12)
📚 Learning: 2025-11-24T16:03:45.295Z
Learnt from: CR
Repo: yamcodes/arkenv PR: 0
File: .cursor/rules/arktype.mdc:0-0
Timestamp: 2025-11-24T16:03:45.295Z
Learning: Applies to packages/arkenv/**/*.ts : Leverage ArkType's built-in types (e.g., `string.host`, `number.port`) where possible in environment schemas
Applied to files:
.changeset/humble-lizards-judge.mdpackages/arkenv/src/utils/coerce.tspackages/arkenv/src/utils/coerce.test.ts
📚 Learning: 2025-09-10T19:35:18.179Z
Learnt from: yamcodes
Repo: yamcodes/arkenv PR: 136
File: .changeset/vast-bananas-win.md:2-3
Timestamp: 2025-09-10T19:35:18.179Z
Learning: The arkenv package is currently in v0.x.x (pre-1.0) development phase, where breaking changes are acceptable in minor version bumps according to semantic versioning conventions.
Applied to files:
.changeset/humble-lizards-judge.md
📚 Learning: 2025-12-22T19:44:07.593Z
Learnt from: yamcodes
Repo: yamcodes/arkenv PR: 596
File: examples/basic/index.ts:4-5
Timestamp: 2025-12-22T19:44:07.593Z
Learning: In examples/basic/index.ts: Use explicit ArkType syntax (e.g., "string.ip | 'localhost'", "0 <= number.integer <= 65535") instead of built-in validators (string.host, number.port) to showcase ArkType's type system capabilities for educational purposes.
Applied to files:
.changeset/humble-lizards-judge.mdpackages/arkenv/src/utils/coerce.test.ts
📚 Learning: 2025-11-24T16:03:45.295Z
Learnt from: CR
Repo: yamcodes/arkenv PR: 0
File: .cursor/rules/arktype.mdc:0-0
Timestamp: 2025-11-24T16:03:45.295Z
Learning: Applies to packages/arkenv/**/*.ts : Use union types for enums in ArkType schemas (e.g., `"'dev' | 'prod'"`) instead of separate enum definitions
Applied to files:
.changeset/humble-lizards-judge.mdpackages/arkenv/src/utils/coerce.tspackages/arkenv/src/utils/coerce.test.ts
📚 Learning: 2025-11-24T16:03:45.295Z
Learnt from: CR
Repo: yamcodes/arkenv PR: 0
File: .cursor/rules/arktype.mdc:0-0
Timestamp: 2025-11-24T16:03:45.295Z
Learning: Applies to packages/arkenv/**/*.ts : Keep environment variable schemas readable and TypeScript-like using ArkType syntax
Applied to files:
.changeset/humble-lizards-judge.mdpackages/arkenv/src/utils/coerce.tspackages/arkenv/src/utils/coerce.test.ts
📚 Learning: 2025-11-24T16:04:00.957Z
Learnt from: CR
Repo: yamcodes/arkenv PR: 0
File: .cursor/rules/coding-guidelines.mdc:0-0
Timestamp: 2025-11-24T16:04:00.957Z
Learning: Applies to **/*.{ts,tsx} : Prefer `Number.parseInt` over global `parseInt` (`useNumberNamespace` error)
Applied to files:
packages/internal/keywords/src/index.ts
📚 Learning: 2025-11-24T16:03:45.295Z
Learnt from: CR
Repo: yamcodes/arkenv PR: 0
File: .cursor/rules/arktype.mdc:0-0
Timestamp: 2025-11-24T16:03:45.295Z
Learning: Applies to packages/arkenv/**/*.ts : Leverage ArkType's type inference for TypeScript types instead of manual type definitions
Applied to files:
packages/arkenv/src/utils/coerce.ts
📚 Learning: 2025-11-24T16:03:45.295Z
Learnt from: CR
Repo: yamcodes/arkenv PR: 0
File: .cursor/rules/arktype.mdc:0-0
Timestamp: 2025-11-24T16:03:45.295Z
Learning: Applies to packages/arkenv/**/*.ts : Convert ArkType validation errors to `ArkEnvError` for user-friendly error messages that include variable name and expected type
Applied to files:
packages/arkenv/src/utils/coerce.tspackages/arkenv/src/utils/coerce.test.ts
📚 Learning: 2025-11-29T22:11:39.931Z
Learnt from: CR
Repo: yamcodes/arkenv PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T22:11:39.931Z
Learning: Applies to **/*.{ts,tsx} : Environment schema definitions should use built-in validators, ArkType string literals, and support default values in the schema pattern
Applied to files:
packages/arkenv/src/utils/coerce.test.ts
📚 Learning: 2025-11-29T22:11:39.931Z
Learnt from: CR
Repo: yamcodes/arkenv PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T22:11:39.931Z
Learning: Applies to **/*.test.{ts,tsx} : Test edge cases including invalid and missing environment variable values
Applied to files:
packages/arkenv/src/utils/coerce.test.ts
📚 Learning: 2025-11-24T16:03:45.295Z
Learnt from: CR
Repo: yamcodes/arkenv PR: 0
File: .cursor/rules/arktype.mdc:0-0
Timestamp: 2025-11-24T16:03:45.295Z
Learning: Applies to packages/arkenv/**/*.ts : Use ArkType's `type()` function to define schemas in environment variable definitions
Applied to files:
packages/arkenv/src/utils/coerce.test.ts
📚 Learning: 2025-11-24T16:03:45.295Z
Learnt from: CR
Repo: yamcodes/arkenv PR: 0
File: .cursor/rules/arktype.mdc:0-0
Timestamp: 2025-11-24T16:03:45.295Z
Learning: ArkType validates environment variables at runtime and TypeScript types are inferred from the schema definition
Applied to files:
packages/arkenv/src/utils/coerce.test.ts
🧬 Code graph analysis (1)
packages/arkenv/src/utils/coerce.test.ts (2)
packages/arkenv/src/type.ts (1)
type(3-3)packages/arkenv/src/utils/coerce.ts (1)
coerce(215-238)
🔇 Additional comments (5)
.changeset/humble-lizards-judge.md (1)
9-9: LGTM!The changelog entry accurately documents the NaN support addition.
packages/arkenv/src/utils/coerce.ts (3)
115-121: LGTM - NaN propagation enabled for root data coercion.The removal of the NaN guard correctly allows NaN values to propagate as valid numbers. Schema validation will reject NaN where appropriate (e.g., plain
numberkeyword).
144-156: LGTM - NaN propagation enabled for array item coercion.Consistent with the root data coercion approach, NaN values are now properly propagated for array items.
164-173: LGTM - NaN propagation enabled for object property arrays.The change maintains consistency across all coercion paths.
packages/arkenv/src/utils/coerce.test.ts (1)
205-214: LGTM - Excellent test coverage for NaN rejection.This test correctly verifies that the plain
numberkeyword rejects NaN values even after coercion, which is the expected behavior. The error message validation ensures proper user feedback.
… by using a single trimmed variable
1 participant
Closes #599
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.