feat: enhance isEmpty function #5
Conversation
WalkthroughRefactors isEmpty with explicit per-type emptiness rules (primitives, boxed, arrays, typed arrays, Maps/Sets, plain objects), uses Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant isEmpty as isEmpty()
participant getTag as getTag()
participant isPlain as isPlainObject()
Caller->>isEmpty: isEmpty(value)
alt value is null/undefined
isEmpty-->>Caller: true
else
isEmpty->>getTag: tag = getTag(value)
alt tag is String
isEmpty-->>Caller: return trimmedString === ''
else tag is Number
isEmpty-->>Caller: return Number.isNaN(value)
else tag is Boolean|BigInt|Symbol|Function
isEmpty-->>Caller: false
else tag is Map|Set
isEmpty-->>Caller: return value.size === 0
else tag is Array or TypedArray
isEmpty-->>Caller: return value.length === 0
else tag is Object
isEmpty->>isPlain: isPlainObject(value)?
alt plain
isPlain-->>isEmpty: yes
isEmpty-->>Caller: return Object.keys(value).length === 0
else non-plain
isPlain-->>isEmpty: no
isEmpty-->>Caller: throw TypeError(tag)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (8)
src/isPlainObject.ts (1)
14-15: Treating null-prototype objects as plain is good; consider cross-realm robustness and avoid double getPrototypeOf.Current check fails for plain objects coming from another realm (e.g., iframes) because their
Object.prototypeis not strictly equal to this realm’sObject.prototype. A robust pattern is to accept objects whose prototype isnullOR whose prototype’s prototype isnull(i.e., some realm’sObject.prototype). Also, compute the prototype once.Apply this diff:
export function isPlainObject( value: unknown, ): value is Record<string, unknown> { - return ( - typeof value === 'object' && - value !== null && - !Array.isArray(value) && - (Object.getPrototypeOf(value) === Object.prototype || - Object.getPrototypeOf(value) === null) - ); + if (typeof value !== 'object' || value === null || Array.isArray(value)) { + return false; + } + const proto = Object.getPrototypeOf(value as object); + // Accept: null-proto objects and objects whose proto is a realm's Object.prototype + return proto === null || Object.getPrototypeOf(proto) === null; }src/isEmpty.ts (4)
61-63: Whitespace-only strings: consider explicit length check (optional).
trim() === ''is fine. If you want to emphasize intent and avoid accidental truthiness pitfalls,trim().length === 0can be a tiny readability win. No need to change if you prefer current style.
71-73: Unify wrapper detection with getTag; avoid realm pitfalls for BigInt (optional).
instanceof BigIntcan be inconsistent across realms. Since you already depend ongetTag, consider using it for wrapper detection and moveconst tag = getTag(value);earlier so it’s available here.Apply this diff:
export function isEmpty(value: unknown): boolean { if (value === undefined || value === null) { return true; } + // Compute once to support wrapper objects and cross-realm cases consistently. + const tag = getTag(value); + if (typeof value === 'boolean' || value instanceof Boolean) { return false; } @@ - if (typeof value === 'bigint' || value instanceof BigInt) { + // Treat all BigInt values (primitive or wrapper) as non-empty data. + if (typeof value === 'bigint' || tag === '[object BigInt]') { return false; } @@ - const tag = getTag(value); + // tag already computed aboveAlso applies to: 83-84
75-77: Map/Set: add tag-based fallback for cross-realm collections (optional).
instanceofmay fail across realms. Sincetagis available, you can tighten this up.- if (value instanceof Map || value instanceof Set) { - return value.size === 0; - } + if (tag === '[object Map]' || tag === '[object Set]') { + return (value as Map<unknown, unknown> | Set<unknown>).size === 0; + }
93-96: Use tag for Symbol wrappers to avoid realm quirks.Replace
instanceof Symbolwith a tag check to reliably classifyObject(Symbol()).- if (typeof value === 'symbol' || value instanceof Symbol) { + if (typeof value === 'symbol' || tag === '[object Symbol]') { return false; }tests/isEmpty.test.ts (3)
83-104: Remove duplicated typed array entries to speed up the suite without losing coverage.You list each typed array twice. Deduplicate to keep tests lean.
Apply this diff:
- it.each([ - ['Uint8Array', Uint8Array], - ['Uint8Array', Uint8Array], - ['Int8Array', Int8Array], - ['Int8Array', Int8Array], - ['Uint16Array', Uint16Array], - ['Uint16Array', Uint16Array], - ['Int16Array', Int16Array], - ['Int16Array', Int16Array], - ['Uint32Array', Uint32Array], - ['Uint32Array', Uint32Array], - ['Int32Array', Int32Array], - ['Int32Array', Int32Array], - ['Float32Array', Float32Array], - ['Float32Array', Float32Array], - ['Float64Array', Float64Array], - ['Float64Array', Float64Array], - ['BigInt64Array', BigInt64Array], - ['BigInt64Array', BigInt64Array], - ['BigUint64Array', BigUint64Array], - ['BigUint64Array', BigUint64Array], - ])('should check typed array: %s', (_, TypedObject) => { + it.each([ + ['Uint8Array', Uint8Array], + ['Int8Array', Int8Array], + ['Uint16Array', Uint16Array], + ['Int16Array', Int16Array], + ['Uint32Array', Uint32Array], + ['Int32Array', Int32Array], + ['Float32Array', Float32Array], + ['Float64Array', Float64Array], + ['BigInt64Array', BigInt64Array], + ['BigUint64Array', BigUint64Array], + ])('should check typed array: %s', (_, TypedObject) => { expect(isEmpty(new TypedObject())).toBe(true); expect(isEmpty(new TypedObject(2))).toBe(false); });
23-31: Add a String wrapper whitespace case for completeness (optional).You cover
new String(''); add a whitespace-only wrapper to mirror the primitive case.it('should return true for empty strings', () => { expect(isEmpty('')).toBe(true); expect(isEmpty(' ')).toBe(true); expect(isEmpty('\n\t')).toBe(true); expect(isEmpty('\u00A0\u2007\u202F\u3000')).toBe(true); // Unicode spaces expect(isEmpty(new String(''))).toBe(true); + expect(isEmpty(new String(' '))).toBe(true); });
172-174: Consider converting the TODO into explicit assertions (optional).If your intent is to throw for non-primitive built-ins (Date, Error, RegExp, Promise, ArrayBuffer, DataView), codify that now for clearer guarantees.
Example additions:
describe('edge cases', () => { @@ - it.todo( - 'Decide: if non-primitive built-ins should be included: Date, Error, RegExp, Promise...', - ); + it('should throw for non-primitive built-ins not explicitly supported', () => { + expect(() => isEmpty(new Date())).toThrow(TypeError); + expect(() => isEmpty(/x/)).toThrow(TypeError); + expect(() => isEmpty(Promise.resolve())).toThrow(TypeError); + expect(() => isEmpty(new ArrayBuffer(0))).toThrow(TypeError); + expect(() => isEmpty(new DataView(new ArrayBuffer(8)))).toThrow(TypeError); + });If you’d like, I can push a commit updating tests and the implementation if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/isEmpty.ts(1 hunks)src/isPlainObject.ts(1 hunks)tests/isEmpty.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/isEmpty.test.ts (1)
src/isEmpty.ts (1)
isEmpty(52-102)
src/isEmpty.ts (1)
src/isPlainObject.ts (1)
isPlainObject(7-17)
🔇 Additional comments (8)
src/isEmpty.ts (7)
7-48: Docs are clear and helpful—nice articulation of emptiness vs. falsiness.The rationale and examples will reduce misuse. Good callouts on symbol-only and non-enumerable-only objects being “empty” by design.
57-60: Boolean handling is correct and explicit.Covers both primitives and wrappers.
65-69: Number handling is solid.Using
value.valueOf()correctly normalizes wrappers; onlyNaNis treated as empty.
79-81: Array handling is straightforward and correct.Early return keeps the typed-array regex from matching plain arrays.
85-87: Typed array detection is concise and correct.Regex covers all typed arrays; using
.lengthis appropriate.
89-91: Plain object emptiness aligns with documented semantics.Using
Object.keysmeans symbol-only and non-enumerable-only objects are “empty,” matching the docs and tests.
101-101: Throwing TypeError for unsupported types is fine.Consistent with tests (e.g., WeakMap/WeakSet). Keep as-is.
tests/isEmpty.test.ts (1)
3-4: Verified path alias configuration: The@/*alias is correctly mapped in bothtsconfig.json(paths:"@/*": ["./src/*"]) andjest.config.mjs(moduleNameMapper:'^@/(.*)$': '<rootDir>/src/$1'). Tests importing@/isEmptywill resolve successfully.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
package.json (1)
58-58: Engines vs tsconfig baseline mismatch; align Node target.You extend @tsconfig/node22 but advertise "node": ">=18". Either raise engines or relax the TS baseline to reduce runtime mismatch risk.
Apply one of:
Option A (prefer): bump engines to Node 22+
- "node": ">=18", + "node": ">=22",Option B: target Node 18 in TS (paired with rollup transpilation)
- "extends": "@tsconfig/node22/tsconfig.json", + "extends": "@tsconfig/node18/tsconfig.json",tsconfig.json (1)
37-40: DisablepreserveConstEnums(noconst enumusage detected)A search for
const enumdeclarations across yoursrc/**/*.tsfiles returned no matches, so disabling this option will have no impact on your current code. Inlining enum values is generally safer for downstream consumers that don’t compile withtsc.• Search performed:
rg -nP '^\s*const\s+enum\s+' -g 'src/**/*.ts' || true--- tsconfig.json @@ -37,4 +37,4 @@ "removeComments": true, "verbatimModuleSyntax": true, - "preserveConstEnums": true, + "preserveConstEnums": false, "forceConsistentCasingInFileNames": trueeslint.config.mjs (1)
29-34: Replace core no-undef/no-shadow with TS-aware configuration.Core rules cause false positives in TS (types-as-values, enum shadowing). Prefer TS-ESLint equivalents and/or scope by file type.
Within this block, remove the core rules:
- 'no-shadow': 'error', - 'no-undef': 'error',Add a TS-only override to re-enable the TS-aware versions:
{ + files: ['**/*.ts'], + rules: { + 'no-shadow': 'off', + 'no-undef': 'off', + '@typescript-eslint/no-shadow': 'warn' + } + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
eslint.config.mjs(1 hunks)package.json(2 hunks)tsconfig.json(1 hunks)
🔇 Additional comments (8)
package.json (2)
20-20: check-types remains a pure type check (good).With tsconfig’s "noEmit": true, this script won’t emit artifacts. Intent matches the script name.
21-22: Repo-wide linting is fine; dist is already ignored in eslint config.No action needed.
tsconfig.json (2)
43-44: Excluding dist is correct for editor performance and lint noise.
12-15: Bundler moduleResolution with isolatedModules is safe for testsI’ve checked that:
- The project uses ts-jest (v29.4.1) with the “default-esm” preset and
useESM: trueinjest.config.mjs, so ts-jest is already set up for ESM resolution against your root tsconfig.json’s settings.- A grep for any
require(...),module.exports, orexports.insrc/returned no CommonJS patterns.Since TypeScript 5.9 (which ts-jest uses) natively supports
"moduleResolution": "Bundler"and there’s no CJS interop in your source, the existing test setup will compile modules correctly without further tweaks. You can consider this concern addressed.eslint.config.mjs (4)
8-10: Base presets selection looks good.
20-25: Downgrading some TS-ESLint rules to warnings for DX is reasonable.
129-136: mjs override to disable type-checked rules is appropriate.
138-144: No action needed: Jest globals are already imported
All.test.tsfiles explicitly import Jest globals (e.g.import { describe, expect, it } from '@jest/globals';), so ESLint’s coreno-undefrule will not report undefined Jest globals. You can leave the existing ESLint configuration for tests as-is.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rollup.config.mjs (1)
6-6: Fix CI lint error: add a type annotation for the Rollup configESLint warns “Expected config to have a type annotation.” Add a JSDoc type.
Apply:
-const config = [ +/** @type {import('rollup').RollupOptions[]} */ +const config = [
♻️ Duplicate comments (1)
package.json (1)
31-50: Jest 30 + ts-jest 29 are incompatible; align majorsts-jest v29 targets Jest 29. Either downgrade Jest to ^29 or upgrade/replace ts-jest.
Two options:
- Downgrade Jest:
- "jest": "^30.0.5", + "jest": "^29.7.0",
- Or upgrade ts-jest (when a Jest 30–compatible release is available) or switch to babel-jest.
Optionally verify and surface the config:
#!/bin/bash set -euo pipefail echo "jest:" && jq -r '.devDependencies.jest' package.json echo "ts-jest:" && jq -r '.devDependencies["ts-jest"]' package.json fd -a 'jest.config.*' -E node_modules | xargs -I{} sh -c 'echo "--- {}"; cat {}'
🧹 Nitpick comments (3)
rollup.config.mjs (1)
28-29: Consider externalizing deps and Node built-insFor libs, leaving external: [] can accidentally bundle future deps and built-ins. Recommend deriving externals from package.json and builtinModules.
Apply:
- external: [], + external: external,Add at the top of the file (outside the selected range):
// Top-level additions to support external inference import { builtinModules } from 'node:module'; import pkg from './package.json' assert { type: 'json' }; const external = [ ...builtinModules, ...builtinModules.map((m) => `node:${m}`), ...Object.keys(pkg.dependencies ?? {}), ...Object.keys(pkg.peerDependencies ?? {}), ];package.json (2)
17-17: Revert to noEmit for check-types (avoid stray build artifacts)Type-check steps typically shouldn’t emit files; rollup handles builds.
Apply:
-"check-types": "tsc --pretty --project tsconfig.json", +"check-types": "tsc --pretty --noEmit --project tsconfig.json",
6-12: Add a top-level “types” for wider tooling compatibilityMany tools resolve types from the top-level field; keeping it alongside exports is harmless and improves DX.
Apply:
"exports": { ".": { "import": "./dist/index.mjs", "require": "./dist/index.cjs", "types": "./dist/index.d.ts" } }, + "types": "./dist/index.d.ts",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
package.json(3 hunks)rollup.config.mjs(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
rollup.config.mjs
[warning] 6-6: ESLint: Expected config to have a type annotation. (typescript-eslint/typedef)
🔇 Additional comments (6)
rollup.config.mjs (2)
11-14: ESM output path switch looks goodChanging ES module output to dist/index.mjs aligns with package.json exports.
33-34: dts plugin simplification is finePlain dts() keeps things maintainable and matches the single types output.
package.json (4)
8-11: Exports import path update is correctdist/index.mjs matches the ESM output in rollup.
18-19: Lint script changes look goodRunning eslint on the repo root is appropriate with flat config.
55-57: Engines update is sensibleNode >=18 aligns with modern ESM and toolchain requirements.
31-50: No changes required: tslib placement is correcttsconfig.json does not enable
importHelpers, sotslibisn’t used at runtime and may remain indevDependencies.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores