Skip to content

refactor: remove lodash dependency - #331

Open
inlined wants to merge 4 commits into
firebase:masterfrom
inlined:remove-lodash-dependency
Open

refactor: remove lodash dependency#331
inlined wants to merge 4 commits into
firebase:masterfrom
inlined:remove-lodash-dependency

Conversation

@inlined

@inlined inlined commented Jul 8, 2026

Copy link
Copy Markdown
Member

Removes lodash and @types/lodash as direct dependencies and replaces all usages with modern native ES6+ features and the existing ts-deepmerge library.

Description

Removes lodash and @types/lodash as direct dependencies and replaces all usages with modern native ES6+ features and the existing ts-deepmerge library.

Scenarios Tested

  • Compiled and ran the unit and integration tests: npm test
  • Validated with ESLint checks: npm run lint

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request removes the dependency on lodash by replacing its utility functions with native ES6+ features, custom lightweight helpers, and ts-deepmerge. The review feedback highlights a few areas of improvement for these custom helpers: updating isPlainObject to support objects created with Object.create(null) for full Lodash compatibility, simplifying mapValues using Object.entries(), and making the in operator check in wrapV1 safer against truthy primitives to prevent runtime TypeErrors.

Comment thread src/providers/firestore.ts
Comment thread src/providers/firestore.ts
Comment thread src/v1.ts Outdated
@cabljac

cabljac commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Nice to see lodash go. Three things from our side before this lands:

  1. Overlap with fix: update firebaseApp check in makeDocumentSnapShot #330: both PRs rewrite the same makeDocumentSnapshot guard (firestore.ts:79-81) and merge-tree confirms a conflict whichever lands second. They agree on the core fix (the has(options, 'app') vs documented firebaseApp bug), but they differ on the undocumented app key: this PR keeps it as a legacy fallback, fix: update firebaseApp check in makeDocumentSnapShot #330 drops it. From what we can see app was never in DocumentSnapshotOptions, so it was only reachable via a cast, and on master it produced a half-broken hybrid (default app's Firestore service with app's projectId in paths), while the both-keys workaround from makeDocumentSnapshot check wrong option names. #79/Updates firebaseApp check in makeDocumentSnapshot #80 keeps working under either version. Given that, is app something you want to support going forward, or should it go while we're touching these lines anyway? Either way, fix: update firebaseApp check in makeDocumentSnapShot #330 carries the regression test for the firebaseApp path, so landing that first and rebasing this one seems like the cheaper order, happy to help with the rebase.

  2. The lodash merge -> ts-deepmerge swap in v1 wrap (v1.ts:149, 201) changes behavior at unchanged call sites: explicitly-undefined option values now wipe generated defaults (lodash skipped undefined sources), e.g. wrap(fn)(data, { eventId: undefined }) loses the generated eventId, and ts-deepmerge silently drops keys named toString/valueOf/etc. Since the v2 mocks already use ts-deepmerge this may be deliberate alignment, in which case a line in the description/changelog would cover it. If lodash fidelity is preferred it reads to me as a one-liner:

    merge.withOptions({ allowUndefinedOverrides: false }, {}, defaultContext, _options)

  3. Two of the bot's inline comments look worth taking: 'in' on a truthy primitive throws a TypeError instead of the friendly "Wrap can only be called on functions..." error (v1.ts:131 and friends), and the custom isPlainObject rejects null-prototype objects so objectToValueProto now throws on maps some parsers produce.

CI is green and the rest of the replacements checked out faithful (random, isEmpty at the live call sites, the resource-name param lookup is actually stricter than lodash get in a good way).

inlined added 2 commits July 27, 2026 13:09
Removes lodash and @types/lodash as direct dependencies and replaces all usages with modern native ES6+ features and the existing ts-deepmerge library.

- Compiled and ran the unit and integration tests: npm test
- Validated with ESLint checks: npm run lint
@inlined
inlined force-pushed the remove-lodash-dependency branch from c243820 to 7a78706 Compare July 27, 2026 20:36
@inlined

inlined commented Jul 27, 2026

Copy link
Copy Markdown
Member Author

Thanks @cabljac! We've addressed your feedback:

  1. Overlap with fix: update firebaseApp check in makeDocumentSnapShot #330: We rebased the branch on upstream master. Since fix: update firebaseApp check in makeDocumentSnapShot #330 already landed and removed the check for the undocumented legacy app property, we resolved the merge conflict by keeping master's implementation (retaining firebaseApp and dropping app entirely).
  2. ts-deepmerge allowUndefinedOverrides: We updated the merge calls in v1.ts to use merge.withOptions({ allowUndefinedOverrides: false }, ...) so that explicitly-undefined option values do not wipe out generated defaults, preserving original lodash behavior.
  3. Safe in and isPlainObject: We incorporated the bot's feedback: refactored isPlainObject to support null-prototype objects, simplified mapValues to use Object.entries(), and safely guarded all in checks on cloudFunction against primitives.

All local tests compile and pass successfully, and all CI checks on GitHub have passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants