-
Notifications
You must be signed in to change notification settings - Fork 356
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
Update SSRC to use validator instead of lodash for non-dev environment #2523
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I missed anything but the only usage of lodash
in #2520 is isString
. Could we use the existing isString()
from the validator
util instead?
export function isString(value: any): value is string { |
Thank you. Yes, we can use that. I've updated the PR with that change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! LGTM!
Discussion
Our tests have used lodash for awhile. PR #2520 added a usage outside of tests. When I run a test server that depends on the integration branch, I get a
Error: Cannot find module 'lodash'
error. So, I think we need to move lodash fromdevDependencies
todependencies
.Moving lodash to
dependencies
and updating the test server to depend on this branch fixes the issue.Perhaps interesting: I wasn't able to repro this issue by making the test server depend on a local copy of the firebase-admin-node, presumably because all dependencies end up in
node_modules
so lodash was available.Note: based on review discussion, this change ended up replacing lodash's
isString
withvalidator.isString
.Testing
Unit tested by running
npm test
on this branch.Functionally tested by updating a test server to depend on this branch and observing it was able to start without error.
API Changes
None