feat: introduce array utilities#74
Conversation
🦋 Changeset detectedLatest commit: 4fdf371 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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. 📝 WalkthroughWalkthroughAdds a new package Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✏️ Tip: You can disable this entire section by setting 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 |
Summary of ChangesHello @teneplaysofficial, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
coderabbitai
Bot
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new @js-utils-kit/array package with four utility functions: chunk, compact, lastElement, and unique, along with their corresponding tests and configuration. The changes are well-structured.
My review focuses on improving performance, type safety, and correctness of the new utilities.
- The
chunkfunction's recursive implementation could lead to performance issues and stack overflows with large arrays; an iterative approach is recommended. - The
compactfunction can be improved to provide better type-narrowing for TypeScript users. - The JSDoc for
lastElementcontains a small error in the example. - The
uniquefunction's documentation and tests are misleading regarding its behavior with object arrays, which could lead to incorrect usage.
Overall, this is a great addition. Addressing these points will make the new array utilities more robust and developer-friendly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.changeset/little-queens-dream.md:
- Around line 1-5: The changeset only documents lastElement but the PR also adds
compact and unique (chunk is in another changeset); update this changeset to
include entries for compact and unique (or create separate changesets for each)
and ensure the package key ('@js-utils-kit/array') and version bump reflect all
added utilities; reference the new utilities by name (lastElement, compact,
unique) in the changeset description and confirm the changeset YAML lists them
so release tooling picks them up.
In `@packages/array/src/lastElement.ts`:
- Around line 6-12: Update the JSDoc examples to call the actual function name
lastElement() instead of last(); change occurrences like last([1, 2, 3]) and
last([]) to lastElement([1, 2, 3]) and lastElement([]) in the comment block for
the lastElement function so the documentation matches the implementation.
In `@packages/array/test/unique.test.ts`:
- Around line 23-41: The test for unique with a custom compareFn is invalid
because unique relies on Set semantics (reference equality) for objects, so the
four objects in users are distinct references; fix by making the duplicate entry
the same object reference (e.g., reuse the { id: 1, name: 'Alice' } instance for
both positions) so Set will dedupe, or alternatively update the expected array
to include all four original objects; locate the test using the unique function
and compareFn and either reuse the same object reference for the duplicate or
change the expected result to match Set behavior.
🧹 Nitpick comments (5)
packages/array/src/compact.ts (1)
17-24: Consider narrowing the return type to exclude falsy values.The current implementation returns
T[], but ideally it should narrow the type to exclude falsy values. For example,compact<string | null>([...])returns(string | null)[]instead ofstring[].This is a known TypeScript limitation with
filter(Boolean). You can improve type safety with a type predicate:♻️ Suggested improvement for type narrowing
+type Truthy<T> = T extends false | 0 | '' | null | undefined ? never : T; + export function compact<T>( /** * A list of elements to compact */ array: readonly T[], -): T[] { - return array.filter(Boolean); +): Truthy<T>[] { + return array.filter(Boolean) as Truthy<T>[]; }packages/array/src/unique.ts (1)
54-61: Defaultsort()may yield unexpected results for numeric arrays.When
sort: trueis passed without acompareFn, the defaultArray.prototype.sort()converts elements to strings, causing lexicographic ordering. For example,unique([10, 2, 1], { sort: true })returns[1, 10, 2]instead of[1, 2, 10].Consider documenting this behavior or providing a numeric-aware default for number arrays.
📝 Option: Add a note in the JSDoc
/** * Whether to sort the result. * * `@default` false + * `@remarks` When sorting without a `compareFn`, elements are sorted lexicographically (converted to strings). + * For numeric arrays, provide a `compareFn` like `(a, b) => a - b` for numeric ordering. */ sort?: boolean;packages/array/package.json (2)
31-36: Consider addingtypesto the exports map for better TypeScript resolution.The exports map only specifies
requireandimportbut omits explicittypesentries. Modern TypeScript tooling (especially withmoduleResolution: "bundler"or"node16") relies on the exports map to resolve type declarations.♻️ Suggested improvement
"exports": { ".": { + "types": "./dist/index.d.ts", "require": "./dist/index.cjs", "import": "./dist/index.mjs" } },Alternatively, if tsdown generates separate
.d.cts/.d.mtsfiles, use conditional types:"exports": { ".": { "require": { "types": "./dist/index.d.cts", "default": "./dist/index.cjs" }, "import": { "types": "./dist/index.d.mts", "default": "./dist/index.mjs" } } }
24-26: Node 22 requirement may limit adoption.Node 20 LTS is still actively supported. Unless the package uses Node 22-specific features, consider relaxing this to
>=20to support more users.packages/array/src/chunk.ts (1)
25-28: Recursive implementation risks stack overflow on large arrays.The recursive approach creates
O(n/size)stack frames. For large arrays with small chunk sizes (e.g., 100,000 elements withsize=1), this will exceed the call stack limit and throw aRangeError: Maximum call stack size exceeded.An iterative approach would be more robust:
♻️ Iterative implementation
export function chunk<T>( array: readonly T[], size: number = 1, ): T[][] { if (size <= 0) throw new RangeError('chunk size must be greater than 0'); if (array.length === 0) return []; - return [array.slice(0, size), ...chunk(array.slice(size), size)]; + const result: T[][] = []; + for (let i = 0; i < array.length; i += size) { + result.push(array.slice(i, i + size)); + } + return result; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.changeset/honest-turtles-dance.md.changeset/little-queens-dream.md.changeset/ninety-wasps-camp.md.changeset/strict-ties-fly.mdpackages/array/package.jsonpackages/array/src/chunk.tspackages/array/src/compact.tspackages/array/src/index.tspackages/array/src/lastElement.tspackages/array/src/unique.tspackages/array/test/chunk.test.tspackages/array/test/compact.test.tspackages/array/test/lastElement.test.tspackages/array/test/unique.test.tspackages/array/tsconfig.jsonpackages/array/tsdown.config.tspackages/core/package.jsonpackages/core/src/index.tstypedoc.json
🧰 Additional context used
🧬 Code graph analysis (4)
packages/array/test/unique.test.ts (1)
packages/array/src/unique.ts (1)
unique(33-61)
packages/array/test/compact.test.ts (1)
packages/array/src/compact.ts (1)
compact(17-24)
packages/array/test/lastElement.test.ts (1)
packages/array/src/lastElement.ts (1)
lastElement(13-20)
packages/array/test/chunk.test.ts (1)
packages/array/src/chunk.ts (1)
chunk(15-29)
🪛 GitHub Actions: CI
packages/array/test/unique.test.ts
[error] 36-36: Vitest: Test failed in 'uses a custom compare function when provided'. AssertionError: expected [ { id: 1, name: 'Alice' }, …(3) ] to deeply equal [ { id: 1, name: 'Alice' }, …(2) ]
🪛 GitHub Check: tests
packages/array/test/unique.test.ts
[failure] 36-36: test/unique.test.ts > uses a custom compare function when provided
AssertionError: expected [ { id: 1, name: 'Alice' }, …(3) ] to deeply equal [ { id: 1, name: 'Alice' }, …(2) ]
- Expected
- Received
@@ -2,10 +2,14 @@
{
"id": 1,
"name": "Alice",
},
{
-
"id": 1, -
"name": "Alice", - },
- {
"id": 2,
"name": "Bob",
},
{
"id": 3,
❯ test/unique.test.ts:36:18
🔇 Additional comments (15)
packages/array/tsconfig.json (1)
1-4: LGTM!The TypeScript configuration is minimal and correctly extends the base config while including the appropriate directories.
.changeset/strict-ties-fly.md (1)
1-5: LGTM!The changeset correctly documents the
chunkutility with a clear description emphasizing the immutable input handling.packages/array/src/lastElement.ts (1)
13-20: LGTM!The implementation using
array.at(-1)is clean and modern. The return type correctly includesundefinedfor empty array cases, and the function properly accepts readonly arrays.packages/array/src/unique.ts (1)
1-32: Well-documented API with clear examples.The JSDoc documentation is comprehensive with useful examples covering primitives, objects, and the sorting options. The reference equality caveat for objects is appropriately noted.
.changeset/ninety-wasps-camp.md (1)
1-5: LGTM!Changeset correctly declares a minor version bump for the new
compactutility, following semver conventions for feature additions..changeset/honest-turtles-dance.md (1)
1-5: LGTM!Changeset properly documents the
uniqueutility addition with its optional sorting feature.typedoc.json (1)
11-12: LGTM!The new array package entry point is correctly added to TypeDoc configuration, ensuring the new utilities will be included in the generated documentation.
packages/core/package.json (1)
52-54: LGTM!The workspace dependency for the new array package is correctly added, maintaining consistency with the existing dependency pattern and enabling the core package to re-export the array utilities.
packages/core/src/index.ts (1)
1-1: LGTM!The new re-export follows the established pattern and is correctly placed in alphabetical order among the other package exports.
packages/array/test/compact.test.ts (1)
1-50: LGTM! Comprehensive test coverage.The tests thoroughly cover edge cases including falsy values, immutability, readonly arrays, and truthy objects (like empty arrays which are truthy). Good alignment with the
filter(Boolean)implementation.packages/array/src/index.ts (1)
1-4: LGTM!Clean barrel export consolidating the array utilities into a single public API surface.
packages/array/test/lastElement.test.ts (1)
1-49: LGTM! Thorough test coverage.The tests comprehensively validate the
lastElementbehavior including edge cases for empty arrays,null/undefinedvalues, and reference preservation for objects. Good use of.at(-1)semantics validation.packages/array/tsdown.config.ts (1)
1-16: LGTM!Standard tsdown configuration with sensible defaults for dual ESM/CJS output, minification, and type declarations.
packages/array/test/chunk.test.ts (1)
1-60: Comprehensive test coverage.The test suite thoroughly covers edge cases including non-divisible lengths, error conditions, immutability, and readonly/object inputs.
packages/array/test/unique.test.ts (1)
1-75: Remaining tests are well-structured.The other tests correctly validate
uniquebehavior including preservation of insertion order, immutability, readonly support, and explicit Set semantics for reference equality.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
teneplaysofficial
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/array/src/compact.ts`:
- Line 6: The JSDoc for compact.ts claims falsy values include `NaN` but the
`Falsy` type used by `compact` (the `Falsy` union) does not include `NaN`,
causing a doc/type mismatch; to fix, update the JSDoc to remove `NaN` so the
comment matches the `Falsy` type, or alternatively adjust the `Falsy` type if
you intentionally want `NaN` excluded at the type level; locate the `Falsy` type
and the `compact` function and either remove `NaN` from the descriptive list in
the JSDoc or change the `Falsy` union to reflect `NaN` (note: representing `NaN`
precisely in TypeScript is tricky), keeping the types and docs consistent.
♻️ Duplicate comments (1)
packages/array/src/compact.ts (1)
24-25: Type assertion could be replaced with a type predicate for improved type safety.The use of
as Exclude<T, Falsy>[]relies on a type assertion rather than a type predicate in the filter callback. This was flagged in a previous review and would provide better compile-time guarantees.
🧹 Nitpick comments (2)
packages/array/package.json (1)
31-36: Consider addingtypesconditions to exports for better TypeScript resolution.The
exportsfield is missing explicittypesconditions. While the top-leveltypesfield exists, modern TypeScript withmoduleResolution: "bundler"or"node16"relies on theexportsfield for type resolution. This can cause type resolution issues for consumers.♻️ Suggested improvement
"exports": { ".": { + "types": "./dist/index.d.ts", "require": "./dist/index.cjs", "import": "./dist/index.mjs" } },Note: Ensure
tsdowngenerates the appropriate.d.tsfile, and adjust the path accordingly (e.g.,.d.ctsor.d.mtsbased on your build output).packages/array/src/index.ts (1)
1-1: Consider re-exporting only the types needed by this package.Re-exporting the entire
@js-utils-kit/typespackage pollutes the array package's public API with unrelated types (e.g.,ArchiveFormat,Hour,ArchiverOptions). This can cause confusion for consumers and potential naming collisions in the future.If the intent is to expose
Falsyforcompactusers, consider a targeted re-export:♻️ Suggested improvement
-export * from '@js-utils-kit/types'; +export type { Falsy } from '@js-utils-kit/types';Alternatively, if users need
Falsy, they can import it directly from@js-utils-kit/types.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.changeset/fresh-pens-accept.mdpackages/array/package.jsonpackages/array/src/chunk.tspackages/array/src/compact.tspackages/array/src/index.tspackages/array/src/lastElement.tspackages/array/src/unique.tspackages/array/test/unique.test.tspackages/types/src/Falsy.tspackages/types/src/index.tsturbo.json
💤 Files with no reviewable changes (1)
- turbo.json
✅ Files skipped from review due to trivial changes (1)
- packages/types/src/Falsy.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/array/src/lastElement.ts
- packages/array/src/chunk.ts
- packages/array/src/unique.ts
- packages/array/test/unique.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/array/src/compact.ts (1)
packages/types/src/Falsy.ts (1)
Falsy(13-13)
🔇 Additional comments (4)
packages/types/src/index.ts (1)
5-5: LGTM!The new
Falsytype export is correctly added and maintains the alphabetical ordering of exports in the barrel file..changeset/fresh-pens-accept.md (1)
1-5: LGTM!The changeset correctly documents a minor version bump for the new
Falsytype addition, which is the appropriate semver level for a non-breaking feature addition.packages/array/package.json (1)
24-26: Verify the Node.js engine requirement.Node.js
>=22is quite restrictive. As of early 2026, Node 20 LTS is still widely used. Confirm this constraint is intentional for the package, as it may limit adoption.packages/array/src/index.ts (1)
2-5: LGTM!The local utility re-exports follow a clean barrel pattern and expose the expected array utilities.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.