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

[Feature]: In Node, hijack perf_hooks.performance in addition to global.performance #462

Open
kevinyou opened this issue Jan 17, 2023 · 10 comments

Comments

@kevinyou
Copy link

kevinyou commented Jan 17, 2023

What did you expect to happen? After calling useFakeTimers(), perf_hooks.performance.now() should return a fake time, just like how performance.now() does.

What actually happens 1. perf_hooks.performance.now() is not the same as performance.now(). The latter is correctly hijacked and returns a fake time. 2. perf_hooks.performance.now() returns a negative value -- undefined behavior?

How to reproduce
https://github.com/kevinyou/wtf-performance-now-mocking/tree/main/sinon-fake-timers

var perf_hooks = require('perf_hooks');
var FakeTimers = require("@sinonjs/fake-timers");

console.log(perf_hooks.performance === performance); // true
console.log(perf_hooks.performance.now()); // 36.97510700020939
console.log(performance.now()); // 37.08340699970722

var clock = FakeTimers.install({
    now: Date.now(),
    shouldClearNativeTimers: true,
});
console.log(perf_hooks.performance === performance); // now false
console.log(perf_hooks.performance.now()); // -4217337.086744
console.log(performance.now()); // 0
@fatso83
Copy link
Contributor

fatso83 commented Jan 17, 2023

This is starting to get a bit exotic ... Think you could get away with simply faking the required exports yourself?

@kevinyou
Copy link
Author

kevinyou commented Jan 19, 2023

Yes, as a workaround I did that (in Jest) and it works:

jest.mock('perf_hooks', () => ({
    performance: {
        now: () => global.performance.now(),
    },
}));

But as a user, it was very confusing until I dug deep to the fake-timers source code. The documentation talks about overriding performance.now() but whether this applies to the one in perf_hooks was unclear. Also, it wasn't until Node v16 that performance became a global so older libraries still access it through perf_hooks.

Maybe the documentation could have a disclaimer about performance in perf_hooks, like in the config.toFake row notes.

@benjamingr
Copy link
Member

I think it's probably fine to support it and not a lot of work?

I'm happy to make changes in Node if that'd help

@fatso83
Copy link
Contributor

fatso83 commented Jan 24, 2023

@benjamingr I am at a bit of a loss as to how to do this cleanly without hooking into module loader territory, so if you have a way of doing this cleanly by exposing something in Node, then by all means feel free ❤️

Copy link

stale bot commented Dec 27, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 27, 2023
@fatso83
Copy link
Contributor

fatso83 commented Aug 25, 2024

@benjamingr Know if something for us to hook into will arrive?

@stale stale bot removed the stale label Aug 25, 2024
@dmurvihill
Copy link

dmurvihill commented Oct 25, 2024

+1, this will definitely help users of limiter. Particularly since Jest can no longer easily use @kevinyou's workaround with ESM.

@fatso83
Copy link
Contributor

fatso83 commented Oct 25, 2024

@dmurvihill If you know how we can do this in some reasonable manner, then feel free to supply a PR. I do not know how. To the best of my knowledge, this would venture into module loader territory, like what I am doing in this article.

On the other hand, node:perf_hooks is a Node API, so maybe Node exposes some hook for us to use, but I would not know.

@dmurvihill
Copy link

I ended up moving away from the affected dependency entirely, but if it comes up again I'll come back for another look.

@fatso83
Copy link
Contributor

fatso83 commented Oct 28, 2024

Never got a reply from Node folks on a related question, so this is requiring a deeper look from someone interested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants