Skip to content
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

Merged
merged 1 commit into from
Jan 20, 2022
Merged

Replace ms with faster @lukeed/ms #11236

merged 1 commit into from
Jan 20, 2022

Conversation

Uzlopak
Copy link
Collaborator

@Uzlopak Uzlopak commented Jan 17, 2022

Replace ms with faster @lukeed/ms

@vkarpov15 vkarpov15 changed the base branch from 6.2 to master January 20, 2022 02:15
@vkarpov15 vkarpov15 changed the base branch from master to 6.2 January 20, 2022 02:19
Copy link
Collaborator

@vkarpov15 vkarpov15 left a 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.

@vkarpov15 vkarpov15 added this to the 6.2.0 milestone Jan 20, 2022
@vkarpov15 vkarpov15 merged commit 433af2b into Automattic:6.2 Jan 20, 2022
@sean-daley
Copy link

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.

@vkarpov15
Copy link
Collaborator

@sean-daley you make a fair point. We merged because, although we don't use ms() much, more perf is nice, plus lukeed looks like a reputable dev and we wanted to support this work. Does using this module cause some issues for you?

@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Jan 23, 2022

@lukeed

Fyi

@sean-daley
Copy link

@sean-daley you make a fair point. We merged because, although we don't use ms() much, more perf is nice, plus lukeed looks like a reputable dev and we wanted to support this work. Does using this module cause some issues for you?

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.

@Uzlopak Uzlopak deleted the replace-ms-with-lukeed branch January 24, 2022 02:32
@vkarpov15
Copy link
Collaborator

@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.

vkarpov15 added a commit that referenced this pull request Feb 1, 2022
@vkarpov15 vkarpov15 removed this from the 6.2.0 milestone Feb 1, 2022
vkarpov15 added a commit that referenced this pull request Feb 1, 2022
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.

None yet

3 participants