Skip to content

Import lodash.bindall instead of lodash/* - #822

Merged
roryabraham merged 2 commits into
mainfrom
rory-lodash-bindall-imports
Aug 22, 2026
Merged

Import lodash.bindall instead of lodash/*#822
roryabraham merged 2 commits into
mainfrom
rory-lodash-bindall-imports

Conversation

@roryabraham

@roryabraham roryabraham commented Aug 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Details

Published lib code imported lodash/bindAll and lodash/noop, but package.json only declares lodash.bindall (plus other per-method lodash packages). lodash itself is only a transitive dependency.

Import lodash.bindall directly and inline a local NOOP so the published package does not depend on an undeclared lodash tree. Adds @types/lodash.bindall for typecheck.

Split out of #802.

Related Issues

For #802

Linked E/App PR

Expensify/App#99227

Automated Tests

No new tests. Import path and a local no-op; existing cache and connection-manager suites cover the call sites.

Manual Tests

  1. Open the Expensify App against this Onyx SHA.
  2. Confirm the app boots and Onyx connections still update (open a report, navigate away, come back).
  3. Confirm there are no module-not-found errors for lodash.

Author Checklist

  • I linked the correct issue in the ### Related Issues section above
  • I linked the corresponding Expensify/App PR in the ### Linked E/App PR section above, and verified this change against it (E/App CI passed and manual testing completed)
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari

@roryabraham
fix: import lodash.bindall instead of lodash/*
61d33db 
Stop relying on a transitive lodash package for bindAll and noop
in published lib code.
@roryabraham roryabraham mentioned this pull request Aug 22, 2026
Closed
40 tasks
@roryabraham
fix: use a named noop instead of an empty arrow
f4c1adb 
Satisfies @typescript-eslint/no-empty-function on the native
InstanceSync stub.
@roryabraham

roryabraham commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 22, 2026

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: f4c1adb0e9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@roryabraham
roryabraham marked this pull request as ready for review August 22, 2026 05:22
@roryabraham
roryabraham requested a review from a team as a code owner August 22, 2026 05:22
@melvin-bot
melvin-bot Bot removed the request for review from a team August 22, 2026 05:23
@roryabraham

roryabraham commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

I suggest we ignore the Require Tests check for this PR

@roryabraham
roryabraham merged commit 7e590e7 into main Aug 22, 2026
14 of 16 checks passed
@os-botify

os-botify Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

🚀 Published to npm in 3.0.104 🎉

@melvin-bot melvin-bot Bot added the Emergency label Aug 22, 2026
@melvin-bot

melvin-bot Bot commented Aug 22, 2026

Copy link
Copy Markdown

@roryabraham looks like this was merged without a test passing. These checks had not passed on f4c1adb when it was merged:

Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

roryabraham added a commit that referenced this pull request Aug 22, 2026
@roryabraham
Merge origin/main into rory-eslint-config-expensify-4
82650a8 
Take the landed atomic PRs (#822–#826). Keep this branch's
seatbelt OnyxUtils entries; drop the obsolete no-nested-ternary
allowance now that degradePerformance is flattened.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carlosmiceli carlosmiceli carlosmiceli approved these changes

Assignees

@roryabraham roryabraham

Labels

Emergency

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@roryabraham @carlosmiceli