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.

Sign up for GitHub

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

Jump to bottom

Feat timing utils #43

Draft
wants to merge 1 commit into
base: next
Choose a base branch
from
Draft

Feat timing utils #43

wants to merge 1 commit into from

Conversation

dnlup
Copy link
Contributor

@dnlup dnlup commented Nov 15, 2021 •
edited

TODO:

  • tests
  • readme
  • types
  • benchmarks

Closes #34

@dnlup
Copy link
Contributor Author

dnlup commented Nov 22, 2021

I need to implement tests, but in the meantime this is the gist that I had in mind 😉.

simonecorsi
simonecorsi approved these changes Nov 22, 2021
View reviewed changes
Copy link
Collaborator

@simonecorsi simonecorsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SMGT! GJ 🚀

Should we include some benchmarks to show overhead (if any) ?

@dnlup
Copy link
Contributor Author

dnlup commented Nov 23, 2021

SMGT! GJ 🚀

Should we include some benchmarks to show overhead (if any) ?

Yes, I'll work on it 😉

@dnlup dnlup force-pushed the feat_timing_utils branch 2 times, most recently from da19805 to dc3334f Compare November 26, 2021 12:03
dnlup
dnlup commented Nov 29, 2021
View reviewed changes
lib/nativeTimerifyWrap.js Outdated Show resolved Hide resolved
dnlup
dnlup commented Nov 30, 2021
View reviewed changes
tests/plugin.test.js Outdated Show resolved Hide resolved
dnlup
dnlup commented Nov 30, 2021
View reviewed changes
index.d.ts Outdated Show resolved Hide resolved
simonecorsi
simonecorsi reviewed Nov 30, 2021
View reviewed changes
index.js Outdated Show resolved Hide resolved
@simonecorsi
Copy link
Collaborator

SGTM overall 🚀 Really good job!

I'm just having some doubt about PerformanceObserver we can talk about that on-call is easier 💪

dnlup
dnlup commented Dec 1, 2021
View reviewed changes
index.d.ts Show resolved Hide resolved
@dnlup
Copy link
Contributor Author

dnlup commented Dec 10, 2021

This will sit here for a while, I am also waiting on this PR:

nodejs/node#40625

@simonecorsi simonecorsi self-requested a review May 11, 2022 13:02
@simonecorsi
Copy link
Collaborator

This is staling since 2021, do we want to close this or is still valuable somehow?

@dnlup
Copy link
Contributor Author

dnlup commented Jan 10, 2023

The problem with the timerify function. should be solved (nodejs/node#43330), but I don't think this PR is a priority,. Maybe we could start from scratch if/when needed.

@simonecorsi
Copy link
Collaborator

The problem with the timerify function. should be solved (nodejs/node#43330), but I don't think this PR is a priority,. Maybe we could start from scratch if/when needed.

I agree with you, maybe it would me more difficult updating this after one year than starting from scratch if you still want to add this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antoniomuso antoniomuso Awaiting requested review from antoniomuso

@simonecorsi simonecorsi Awaiting requested review from simonecorsi simonecorsi is a code owner

Assignees
No one assigned
Labels
blocked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add the possibity to track a time execution of async functions
3 participants
@dnlup @simonecorsi @antoniomuso