Open
Conversation
Replace lodash/isEmpty, lodash/pick, lodash/isObject, lodash/merge, and lodash/throttle with lightweight native implementations in a new src/utils/lang.ts module. Remove lodash and @types/lodash from dependencies (~4.4 MB install size reduction). Made-with: Cursor
Renames the new utility module to a more descriptive name, documents each helper with a header block explaining its narrow contract vs lodash, and adds unit tests covering the behaviors the CLI actually depends on (nullish handling, falsy preservation, non-mutation, throttle leading-edge semantics). Intended to set a baseline code-quality example for future utility modules. Made-with: Cursor
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR removes lodash from dependencies and adds Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
📊 Benchmark resultsComparing with cb924d4
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (1)
src/commands/base-command.ts (1)
193-207:⚠️ Potential issue | 🟠 MajorPotential regression: existing
auth.github.{user,token}will now be overwritten withundefinedon every login.
lodash/mergeskipped source properties whose value isundefinedwhen a destination value existed, so the explicitgithub: { user: undefined, token: undefined }block was effectively a no-op when the user already had GitHub auth stored. The newdeepMerge(seesrc/utils/object-utilities.tsL44–63) assignsundefinedunconditionally, which will clear those fields on everystoreTokencall. See the root-cause comment ondeepMergefor a proposed fix to restore lodash semantics.If you prefer to fix it here instead, drop the explicit
undefinedfields:🔧 Alternative: remove the no-op undefineds
const userData = deepMerge(globalConfig.get(`users.${userId}`), { id: userId, name, email, auth: { token: accessToken, - github: { - user: undefined, - token: undefined, - }, }, })(Only safe if clearing those fields was never the intent.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/base-command.ts` around lines 193 - 207, The deepMerge call in base-command.ts is overwriting existing users.${userId}.auth.github.user and .token with explicit undefined values; either fix deepMerge to preserve destination values when source properties are undefined (restore lodash.merge semantics in src/utils/object-utilities.ts) or remove the explicit github: { user: undefined, token: undefined } from the deepMerge payload here (inside the storeToken/update user flow) so existing GitHub auth fields are not cleared when calling deepMerge/globalConfig.set for userData. Ensure the change targets the deepMerge usage or the payload creation around userData to prevent unconditional clearing of auth.github fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/deploy/deploy.ts`:
- Around line 9-13: The import ordering is broken because the isEmpty import is
placed between third-party imports (inquirer and `@netlify/headers-parser`); move
the isEmpty import statement so it sits with the other local utility imports
(e.g., next to the ../../utils/command-helpers.js import) rather than between
third-party modules, then re-run the formatter/Prettier to ensure imports are
grouped and sorted correctly.
In `@src/utils/object-utilities.ts`:
- Around line 1-90: Prettier check failed for this file; run the project
formatter and commit the changes so formatting matches CI. Run the repo's format
script (e.g. npm run format or prettier --write) on
src/utils/object-utilities.ts, then review and stage the updated file (covering
exports isEmpty, pick, deepMerge, throttle) and push the formatted version so
prettier --check passes in CI.
- Around line 44-63: deepMerge currently overwrites existing values with
undefined from the source; update the deepMerge function to mirror lodash
_.merge semantics by skipping assignment when sourceVal is undefined (i.e., if
sourceVal === undefined and result already has a non-undefined value, do not set
result[key] = sourceVal), while preserving the recursive merge path for nested
plain objects; ensure this change fixes the storeToken call site behavior (where
github.user/github.token may be passed as undefined) so existing auth.github.*
values are not wiped out unintentionally.
In `@tests/unit/utils/object-utilities.test.ts`:
- Around line 137-147: The test "drops calls that arrive within the throttle
interval" currently only asserts before the interval expires and could pass with
a trailing-call implementation; update the test to advance timers past the
throttle interval after the extra calls and assert the original fn was not
called again (i.e., call throttle(fn, 100), invoke throttled(), advance 50ms,
invoke throttled() twice, then advance timers by at least the remaining 50ms or
slightly more and assert expect(fn).toHaveBeenCalledTimes(1)) to ensure no
trailing invocation is scheduled by the throttle implementation.
---
Outside diff comments:
In `@src/commands/base-command.ts`:
- Around line 193-207: The deepMerge call in base-command.ts is overwriting
existing users.${userId}.auth.github.user and .token with explicit undefined
values; either fix deepMerge to preserve destination values when source
properties are undefined (restore lodash.merge semantics in
src/utils/object-utilities.ts) or remove the explicit github: { user: undefined,
token: undefined } from the deepMerge payload here (inside the storeToken/update
user flow) so existing GitHub auth fields are not cleared when calling
deepMerge/globalConfig.set for userData. Ensure the change targets the deepMerge
usage or the payload creation around userData to prevent unconditional clearing
of auth.github fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23910191-460e-4845-b600-0822211ac907
📒 Files selected for processing (11)
package.jsonsrc/commands/base-command.tssrc/commands/deploy/deploy.tssrc/commands/init/init.tssrc/commands/link/link.tssrc/commands/sites/sites-create.tssrc/lib/extensions.tssrc/utils/dev.tssrc/utils/object-utilities.tssrc/utils/proxy.tstests/unit/utils/object-utilities.test.ts
💤 Files with no reviewable changes (1)
- package.json
Comment on lines
+1
to
+90
| /** | ||
| * Checks whether a value is effectively empty. | ||
| * | ||
| * A value is considered empty when it is `null`, `undefined`, or an object | ||
| * with no own enumerable keys. Intended as a minimal stand-in for the handful | ||
| * of `lodash/isEmpty` call sites that only ever receive plain objects or | ||
| * nullish values (e.g. API response payloads that may be absent). | ||
| * | ||
| * @param obj - The object to check, or a nullish value. | ||
| * @returns `true` if the value is nullish or has no own enumerable keys. | ||
| */ | ||
| export const isEmpty = (obj: object | null | undefined): boolean => obj == null || Object.keys(obj).length === 0 | ||
|
|
||
| /** | ||
| * Returns a new object containing only the specified keys from the source. | ||
| * | ||
| * Mirrors the shape of `lodash/pick` but is intentionally loosely typed so it | ||
| * can accept string keys that are not declared on the source type. This is | ||
| * pragmatic for CLI output filtering of API responses whose generated types | ||
| * sometimes omit fields the server actually returns. | ||
| * | ||
| * Keys that are not present on the source are silently skipped. | ||
| * | ||
| * @param obj - The source object to pick properties from. | ||
| * @param keys - The property names to include in the result. | ||
| * @returns A new object with only the requested keys that exist on the source. | ||
| */ | ||
| export const pick = <T extends object>(obj: T, keys: readonly string[]): Partial<T> => | ||
| Object.fromEntries(keys.filter((k) => k in obj).map((k) => [k, (obj as Record<string, unknown>)[k]])) as Partial<T> | ||
|
|
||
| /** | ||
| * Recursively merges the properties of `source` into a copy of `target`. | ||
| * | ||
| * Only plain (non-array) objects are merged recursively; arrays and primitive | ||
| * values from `source` overwrite the corresponding value in `target`. Neither | ||
| * argument is mutated. Behavior is intentionally narrow: this is not a full | ||
| * replacement for `lodash/merge`, only enough to cover the existing call sites | ||
| * that merge shallow-nested config objects. | ||
| * | ||
| * @param target - The base object to merge into. `undefined` is treated as `{}`. | ||
| * @param source - The object whose properties take precedence over `target`. | ||
| * @returns A new object containing the combined properties. | ||
| */ | ||
| export function deepMerge<T extends Record<string, unknown>>(target: T | undefined, source: Record<string, unknown>): T { | ||
| const result: Record<string, unknown> = { ...(target ?? {}) } | ||
| for (const key of Object.keys(source)) { | ||
| const targetVal = result[key] | ||
| const sourceVal = source[key] | ||
| if ( | ||
| targetVal != null && | ||
| sourceVal != null && | ||
| typeof targetVal === 'object' && | ||
| typeof sourceVal === 'object' && | ||
| !Array.isArray(targetVal) && | ||
| !Array.isArray(sourceVal) | ||
| ) { | ||
| result[key] = deepMerge(targetVal as Record<string, unknown>, sourceVal as Record<string, unknown>) | ||
| } else { | ||
| result[key] = sourceVal | ||
| } | ||
| } | ||
| return result as T | ||
| } | ||
|
|
||
| /** | ||
| * Wraps a function so that it runs at most once per `intervalMs` milliseconds. | ||
| * | ||
| * Uses leading-edge invocation: the first call runs immediately, and any calls | ||
| * that arrive within `intervalMs` of the previous invocation are dropped. | ||
| * There is no trailing call -- this is intentional for fire-and-forget | ||
| * side-effect callers (e.g. rate-limited activity pings) where losing the | ||
| * final invocation is acceptable. | ||
| * | ||
| * @param fn - The function to throttle. | ||
| * @param intervalMs - Minimum time between invocations, in milliseconds. | ||
| * @returns A throttled wrapper around `fn` with the same arguments signature. | ||
| */ | ||
| export function throttle<Args extends unknown[]>( | ||
| fn: (...args: Args) => void, | ||
| intervalMs: number, | ||
| ): (...args: Args) => void { | ||
| let lastCall = 0 | ||
| return (...args: Args) => { | ||
| const now = Date.now() | ||
| if (now - lastCall >= intervalMs) { | ||
| lastCall = now | ||
| fn(...args) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Prettier --check is failing on this file.
The CI Format job reports formatting issues. Please run npm run format (or equivalent) before pushing so prettier --check passes.
🧰 Tools
🪛 GitHub Actions: Format
[warning] 1-1: Prettier --check reported formatting/style issues in this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/object-utilities.ts` around lines 1 - 90, Prettier check failed for
this file; run the project formatter and commit the changes so formatting
matches CI. Run the repo's format script (e.g. npm run format or prettier
--write) on src/utils/object-utilities.ts, then review and stage the updated
file (covering exports isEmpty, pick, deepMerge, throttle) and push the
formatted version so prettier --check passes in CI.
- deepMerge: skip undefined source values to match lodash.merge semantics. This preserves the storeToken call site in base-command.ts, which passes github.user/github.token as undefined and relied on the existing auth fields being left intact. - Move the misplaced object-utilities import in deploy.ts into the local imports block. - Run Prettier on the affected files to match CI formatting. - Strengthen the throttle test to advance past the interval and assert no trailing invocation is scheduled. - Update deepMerge tests: null still overwrites; undefined no longer does, plus coverage for the storeToken-style nested-undefined patch. Made-with: Cursor
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes
lodashfrom the CLI's runtime dependencies in favor of tiny native implementations insrc/utils/object-utilities.ts.lodash/isEmpty,lodash/pick,lodash/merge,lodash/throttle, andlodash/isObjectacross 8 call sites.lodashand@types/lodashfrompackage.json.tests/unit/utils/object-utilities.test.tscovering nullish handling, falsy-value preservation, non-mutation, recursive merge, array overwrite, and throttle timing behavior (26 tests).Part of the Milestone 1 dependency-pruning work from the CLI modernization RFC.
node-fetchandbackoffare deliberately left for follow-up PRs to keep this one reviewable.Test plan
npm run buildnpx tsc --project tsconfig.build.json --noEmitnpm run test:unit— 344 passed; the 5 failures (exec-fetcher,hash-fns) are pre-existing onmainand unrelated (flaky CDN download in traffic-mesh fetch).npm run test:integration— 545 passed; 5 flaky failures unrelated to this change (interactive completion prompts, Next.js/Hugo fixture tests, a dev plugin-hook test). None touch the lodash replacement call sites.Made with Cursor