-
-
Notifications
You must be signed in to change notification settings - Fork 3.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
Replace ms with faster @lukeed/ms #11236
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.
I took a closer look at the @lukeed/ms repo and we'll merge this into 6.2.
My only concern here is that this change will have negligible impact: we only use ms()
when creating a new schema, which is typically only done when the app is starting up.
Was there a particular motivation for switching from a library with 100 million weekly downloads to a fork of that library with 5000 or so? Unless you really need the performance improvements provided by this (which it doesn’t seem like you use it very much?) that seems like a curious decision to make. |
@sean-daley you make a fair point. We merged because, although we don't use |
Fyi |
No, it doesn’t really cause issues for us I was just more curious at the reasoning for inclusion. (ms) seems to be a pretty popular library and if it’s not really solving a performance issue you’re having, you’re now imposing another dependency on someone who uses both ms and mongoose. Given the similarities between the two it won’t be a huge addition but it’s still “one more library” from an auditing and/or vetting perspective. Regardless it’s not a major issue for us, I was just more curious than anything else. |
@sean-daley you're right about imposing another dependency being a bit of a pain. Nobody wants more dependabot alerts. We'll revert this change for now since we don't need the extra perf. |
Replace ms with faster @lukeed/ms