feat: add deepMerge utility and PlainObject type#129
Conversation
- introduce `deepMerge` for immutable recursive object merging - support array strategies: `replace`, `concat`, `merge` - deprecate `mergeObj` in favor of `deepMerge` - add `PlainObject` type in `@js-utils-kit/types` - update tsconfig config filename and extends paths - remove compilerOptions from `packages/@js-utils-kit` tsconfig files - adjust affected file paths after tsconfig changes
🦋 Changeset detectedLatest commit: 8556835 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
📝 WalkthroughWalkthroughAdds a new deepMerge utility and DeepMergeOptions to the object package, introduces PlainObject type in types, re-exports/deprecates mergeObj, adds tests, updates package deps, and standardizes many package tsconfig extends to the root tsconfig.json. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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, 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 object manipulation capabilities by introducing a new, highly configurable 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. Changelog
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
|
![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 deepMerge utility and a PlainObject type, deprecates mergeObj, and refactors TypeScript configuration files for better maintainability. However, the deepMerge function is vulnerable to prototype manipulation due to a lack of sanitization for sensitive keys like __proto__, which could lead to Denial of Service or unexpected behavior. Additionally, the deepMerge function does not correctly handle properties with symbol keys, contradicting the PlainObject type definition, and there are minor code redundancies that could be improved for clarity.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/new-project.js (2)
64-70:⚠️ Potential issue | 🟡 Minor
value.trim()doesn't reassign the result.
String.trim()returns a new string; it doesn't mutatevalue. A whitespace-only input like" "will pass the!valuecheck but become empty after trimming on line 94.Proposed fix
validate: (value) => { - value.trim(); + value = value.trim(); if (!value) return 'Library name cannot be empty';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/new-project.js` around lines 64 - 70, The validate function in scripts/new-project.js is calling value.trim() without using its result, so whitespace-only names pass validation; change the validator to trim the input and use the trimmed string for subsequent checks and existence tests (e.g., assign trimmed = value.trim() or reassign value = value.trim()), then check if trimmed is empty and call existsSync(join('packages', trimmed)) when validating uniqueness.
79-83:⚠️ Potential issue | 🟡 MinorSame issue:
value.trim()result is discarded.Proposed fix
validate: (value) => { - value.trim(); + value = value.trim(); if (!value) return 'Library description cannot be empty';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/new-project.js` around lines 79 - 83, The validate handler in scripts/new-project.js discards the result of value.trim(), so empty-or-whitespace input passes validation; update the validate function (the validate: (value) => { ... } block) to assign or use the trimmed string (e.g., set value = value.trim() or use const trimmed = value.trim()) and then check that trimmed for emptiness, returning 'Library description cannot be empty' when empty and true when valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/`@js-utils-kit/object/src/deepMerge.ts:
- Around line 22-27: The current isDeepMergeOptions detection treats any plain
object whose keys are a subset of OPTION_KEYS as options, which mistakenly
consumes valid data objects (e.g., { arrayStrategy: 'concat' }); change
detection to require an explicit marker so only intentionally-passed options are
treated as options: add a unique symbol constant (e.g.,
DEEP_MERGE_OPTIONS_SYMBOL) and update isDeepMergeOptions to return true only if
the value is an object and (value has the symbol marker OR its keys are
OPTION_KEYS but also include the marker), then export a small helper (e.g.,
createDeepMergeOptions) that attaches that symbol to options objects; update any
call sites that relied on implicit detection to use the helper or pass options
via the marker so real data objects are no longer misinterpreted (references:
isDeepMergeOptions, OPTION_KEYS, DeepMergeOptions, deepMerge).
- Around line 115-149: The merge currently aliases caller-owned arrays when
incoming is an array but existing is not (the fall-through path that does
result[key] = incoming); update deepMerge so any time incoming is an array you
assign a shallow-cloned array instead of the original (e.g., result[key] =
[...(incoming as unknown[])]) so mutations to the merged result don't affect the
source; keep the existing arrayStrategy logic for when both are arrays and
ensure the object-merge branch still clones arrays when delegating (use the same
spread clone when creating result entries from incoming arrays).
- Around line 105-149: The deepMerge implementation can be exploited via
prototype pollution because it iterates Object.keys(param) and assigns into the
plain result object; update deepMerge to guard against dangerous property names
by (a) creating the accumulator with no prototype (use Object.create(null) for
result) and (b) filtering out keys ['__proto__','constructor','prototype']
before processing each key in the loop (references: deepMerge function, result
variable, params loop and the inner for (const key of Object.keys(param))
block). Ensure both the root result and recursive merges use the same safeguards
so incoming objects cannot mutate prototypes.
In `@packages/`@js-utils-kit/types/src/PlainObject.ts:
- Around line 1-2: deepMerge currently uses Object.keys and drops symbol-keyed
properties even though PlainObject uses PropertyKey (which includes symbols);
update the deepMerge implementation (the deepMerge function) to enumerate source
object keys with Reflect.ownKeys(source) instead of Object.keys(source), iterate
that keys array and handle symbol and string keys the same way you handle string
keys (merge or assign values, recurse for nested objects), ensuring you use the
same type guards/assign logic already present so symbol-keyed properties are
preserved during the merge.
---
Outside diff comments:
In `@scripts/new-project.js`:
- Around line 64-70: The validate function in scripts/new-project.js is calling
value.trim() without using its result, so whitespace-only names pass validation;
change the validator to trim the input and use the trimmed string for subsequent
checks and existence tests (e.g., assign trimmed = value.trim() or reassign
value = value.trim()), then check if trimmed is empty and call
existsSync(join('packages', trimmed)) when validating uniqueness.
- Around line 79-83: The validate handler in scripts/new-project.js discards the
result of value.trim(), so empty-or-whitespace input passes validation; update
the validate function (the validate: (value) => { ... } block) to assign or use
the trimmed string (e.g., set value = value.trim() or use const trimmed =
value.trim()) and then check that trimmed for emptiness, returning 'Library
description cannot be empty' when empty and true when valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5aa8fd8-6c80-42e3-90b6-51a70361c51c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
.changeset/polite-spies-happen.md.changeset/solid-masks-spend.mdexports.jsonpackage.jsonpackages/@js-utils-kit/array/tsconfig.jsonpackages/@js-utils-kit/charset/tsconfig.jsonpackages/@js-utils-kit/constants/tsconfig.jsonpackages/@js-utils-kit/core/tsconfig.jsonpackages/@js-utils-kit/env/tsconfig.jsonpackages/@js-utils-kit/fs/tsconfig.jsonpackages/@js-utils-kit/number/tsconfig.jsonpackages/@js-utils-kit/object/package.jsonpackages/@js-utils-kit/object/src/deepMerge.tspackages/@js-utils-kit/object/src/index.tspackages/@js-utils-kit/object/src/mergeObj.tspackages/@js-utils-kit/object/test/deepMerge.test.tspackages/@js-utils-kit/object/tsconfig.jsonpackages/@js-utils-kit/pm/tsconfig.jsonpackages/@js-utils-kit/string/tsconfig.jsonpackages/@js-utils-kit/system/tsconfig.jsonpackages/@js-utils-kit/types/src/PlainObject.tspackages/@js-utils-kit/types/src/index.tspackages/@js-utils-kit/types/tsconfig.jsonpackages/@js-utils-kit/valid/tsconfig.jsonpackages/js-utils-kit/tsconfig.jsonpackages/juk-cli/tsconfig.jsonscripts/exports.jsscripts/new-project.jstsconfig.json
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
exports.json (1)
181-192:⚠️ Potential issue | 🟠 Major
DeepMergeOptionsis being published with the wrong public contract.Line 181 and Line 1000 expose
DeepMergeOptions, butpackages/@js-utils-kit/object/src/deepMerge.ts, Lines 7-13 currently only allowarrayStrategy?: 'replace' | 'append'. The PR objective says this release should supportreplace,concat, andmerge, so the exported type is already out of sync with the API you're shipping. Consumers will be unable to type valid options from the announced surface.Also applies to: 1000-1005
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@exports.json` around lines 181 - 192, The exported type DeepMergeOptions is out of sync with the implementation in packages/@js-utils-kit/object/src/deepMerge.ts — update the DeepMergeOptions definition to allow arrayStrategy values 'replace' | 'append' | 'concat' | 'merge' (or otherwise align the exported contract with the actual supported strategies) so consumers can type the new options; locate the DeepMergeOptions type in deepMerge.ts and adjust its union to include 'concat' and 'merge' (and ensure the exports.json entries remain consistent).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@exports.json`:
- Around line 992-1013: The deepMerge implementation currently mutates its first
argument and references an undefined variable in the array branch, so don't
deprecate mergeObj yet; update deepMerge (in
packages/@js-utils-kit/object/src/deepMerge.ts, function deepMerge) to create a
fresh result (e.g., clone or shallow-copy params[0] into result) before merging
so no input is mutated, fix the array-branch variable by using the correct
incoming value rather than the undefined `incoming`, and ensure recursive
merging returns new objects/arrays (preserving immutability); once deepMerge
matches mergeObj's behavior and has unit tests covering object/array recursion
and non-mutation, then deprecate mergeObj.
---
Outside diff comments:
In `@exports.json`:
- Around line 181-192: The exported type DeepMergeOptions is out of sync with
the implementation in packages/@js-utils-kit/object/src/deepMerge.ts — update
the DeepMergeOptions definition to allow arrayStrategy values 'replace' |
'append' | 'concat' | 'merge' (or otherwise align the exported contract with the
actual supported strategies) so consumers can type the new options; locate the
DeepMergeOptions type in deepMerge.ts and adjust its union to include 'concat'
and 'merge' (and ensure the exports.json entries remain consistent).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7d9ae2b-514e-4028-ad58-e145c334bbcc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
exports.jsonpackage.json
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/@js-utils-kit/object/src/deepMerge.ts (2)
117-150:⚠️ Potential issue | 🟠 MajorClone incoming arrays before the object branch.
Arrays are only copied when both sides are already arrays. When the incoming value is the first array seen for a key, this path skips the clone, so the merged result can still share caller-owned array state. Add an
isArray(incoming)branch before the object branch.Suggested patch
if (isArray(existing) && isArray(incoming)) { switch (arrayStrategy) { case 'concat': result[key] = [...(existing as unknown[]), ...(incoming as unknown[])]; break; @@ continue; } + if (isArray(incoming)) { + result[key] = [...(incoming as unknown[])]; + continue; + } + // Object if (isObject(incoming)) { result[key] = isObject(existing) && !Array.isArray(existing) ? deepMerge(existing as PlainObject, incoming as PlainObject, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@js-utils-kit/object/src/deepMerge.ts around lines 117 - 150, The merge currently only clones arrays when both existing and incoming are arrays, so when incoming is an array and existing is not the merged result can retain caller-owned array state; before the "Object" branch in deepMerge (inside packages/@js-utils-kit/object/src/deepMerge.ts) add an isArray(incoming) branch that handles cases where existing is not an array by cloning incoming (respecting arrayStrategy: 'concat'/'merge'/'replace' semantics or at minimum copying incoming into result[key] as a new array) so result[key] never references the original incoming array; update logic around existing, incoming, result, arrayStrategy, key and reuse the same switch/handling used for the "Array" branch or a simplified clone when appropriate.
22-28:⚠️ Potential issue | 🔴 CriticalDisambiguate options from merge data.
Any trailing plain object whose keys are limited to
arrayStrategyis still treated as config, so valid payloads likedeepMerge({}, { arrayStrategy: 'concat' })get dropped instead of merged. This makes part of the input space unrepresentable. Please switch to an explicit options marker/helper or a separate options parameter instead of key-based inference.Also applies to: 101-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@js-utils-kit/object/src/deepMerge.ts around lines 22 - 28, The current isDeepMergeOptions implementation (and related checks around OPTION_KEYS) wrongly treats any plain object whose keys match OPTION_KEYS as the options object, causing valid payloads like deepMerge({}, { arrayStrategy: 'concat' }) to be treated as config and dropped; change the API to use an explicit options marker or a separate options parameter: either add a unique property/symbol (e.g. __isDeepMergeOptions or a Symbol) and update isDeepMergeOptions to detect that marker (instead of relying on OPTION_KEYS), or refactor the public deepMerge signature to accept options as a separate last parameter and remove key-based inference; update all references to isDeepMergeOptions, DeepMergeOptions, and OPTION_KEYS accordingly so callers and internal logic unambiguously distinguish options from data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/`@js-utils-kit/object/src/deepMerge.ts:
- Around line 117-150: The merge currently only clones arrays when both existing
and incoming are arrays, so when incoming is an array and existing is not the
merged result can retain caller-owned array state; before the "Object" branch in
deepMerge (inside packages/@js-utils-kit/object/src/deepMerge.ts) add an
isArray(incoming) branch that handles cases where existing is not an array by
cloning incoming (respecting arrayStrategy: 'concat'/'merge'/'replace' semantics
or at minimum copying incoming into result[key] as a new array) so result[key]
never references the original incoming array; update logic around existing,
incoming, result, arrayStrategy, key and reuse the same switch/handling used for
the "Array" branch or a simplified clone when appropriate.
- Around line 22-28: The current isDeepMergeOptions implementation (and related
checks around OPTION_KEYS) wrongly treats any plain object whose keys match
OPTION_KEYS as the options object, causing valid payloads like deepMerge({}, {
arrayStrategy: 'concat' }) to be treated as config and dropped; change the API
to use an explicit options marker or a separate options parameter: either add a
unique property/symbol (e.g. __isDeepMergeOptions or a Symbol) and update
isDeepMergeOptions to detect that marker (instead of relying on OPTION_KEYS), or
refactor the public deepMerge signature to accept options as a separate last
parameter and remove key-based inference; update all references to
isDeepMergeOptions, DeepMergeOptions, and OPTION_KEYS accordingly so callers and
internal logic unambiguously distinguish options from data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6557a07c-15ff-4e1f-8f16-a528fa8e6d11
📒 Files selected for processing (3)
CONTRIBUTING.mdeslint.config.tspackages/@js-utils-kit/object/src/deepMerge.ts
✅ Files skipped from review due to trivial changes (1)
- CONTRIBUTING.md
1 participant
deepMergefor immutable recursive object mergingreplace,concat,mergemergeObjin favor ofdeepMergePlainObjecttype in@js-utils-kit/typesSummary by CodeRabbit
New Features
Deprecations
Tests
Chores