-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[E.cash] - Updated Lodash Dependency to Fix Vulnerability #1947
Conversation
Testing on all platforms now. So far web & mobile web are clear |
All 5 platforms seem stable. I tried to hit all the pages that were using lodash. |
Let's maybe hold this so that you can include the onyx upgrade in it too? |
That's a great point. I'll wait for the onyx one to get deployed and then retest this. 👍 |
Looks like this change made in Onyx: Is already available in Expensify.cash. I'm going hold this other PR on this one. |
Yea, I updated Onyx on monday before I was OOO today. Retesting this now |
Retested on all 5 and it's lookin good. We need to be clever about when this gets merged. It'd be bad if another PR using the wrong lodash package got merged right before this one. For this reason, I think it's best to reassign the reviewer role to someone in my time-zone, so we can be sure to merge this at the right moment. @tgolen or @roryabraham Would either of you be willing to swap in for @Gonals and become the reviewer of this PR? |
Yeah, I don't mind reviewing and merging this. It looks fine! |
This PR is on [HOLD] pending the deployment of this PR.
cc @tgolen
Details
Rather than using individual packages for each lodash method, we are using the main lodash package and selectively loading the methods we need (using this handy syntax:
lodash/orderby
)Fixed Issues
Fixes https://github.com/Expensify/Expensify/issues/157894Tests
Our testing method for this is admittedly a bit primitive; comb the site looking for errors/broken stuff.
Tested On