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

performance still breaks in Node 15 #732

Closed
vrugtehagel opened this issue Aug 15, 2024 · 5 comments · Fixed by KaliforniaShell/docs#5 · 4 remaining pull requests
Closed

performance still breaks in Node 15 #732

vrugtehagel opened this issue Aug 15, 2024 · 5 comments · Fixed by KaliforniaShell/docs#5 · 4 remaining pull requests
Labels

Comments

@vrugtehagel
Copy link
Contributor

A followup on #728.

The PR I opened today (#729) did resolve this issue for Node 14, but unfortunately, Node 15 still throws an error due to the node: prefix in the import.

@vrugtehagel vrugtehagel changed the title Performance still breaks in Node 15 performance still breaks in Node 15 Aug 15, 2024
@vrugtehagel
Copy link
Contributor Author

vrugtehagel commented Aug 15, 2024

Not sure what to do with this, actually, since the test runner doesn't even want to run on Node 15. What do you suggest, @harttle? Happy to open a followup PR with any suggestions you might have.

From the Jest docs:

The supported Node versions are 14.15, 16.10, 18.0 and above.

@mgnsk
Copy link

mgnsk commented Aug 16, 2024

We're experiencing a similar problem not in Node but using LiquidJS as a WASM module compiled with Javy which doesn't seem to have access to monotonic timers out of the box.

The only easy fix I found was to define a globalThis.performance object with a now method that always returns 0 since we're already limiting the run time externally.

@vrugtehagel
Copy link
Contributor Author

vrugtehagel commented Aug 16, 2024

I guess, one solution is to use something like

const startupTime = globalThis.performance ? 0 : (globalThis.Date?.now() ?? 0);
const now = () => {
  if(globalThis.performance){
    return globalThis.performance.now();
  }
  const current = globalThis.Date?.now() ?? 0;
  return current - startupTime;
}

and then we can drop the node:perf_hooks imports altogether. I know Date.now() is not as accurate, but better than nothing anyway. And this also fails gracefully if you are using an environment that has no timers at all.

@harttle
Copy link
Owner

harttle commented Aug 16, 2024

@vrugtehagel agreed. We can fallback to Date.now() for platforms don't provide the performance global variable. Let me work on a fix.

the test runner doesn't even want to run on Node 15

I can run jest locally with Node@15, I'll try include it on CI.

harttle added a commit that referenced this issue Aug 16, 2024
harttle added a commit that referenced this issue Aug 16, 2024
github-actions bot pushed a commit that referenced this issue Aug 16, 2024
## [10.16.3](v10.16.2...v10.16.3) (2024-08-16)

### Bug Fixes

* support for NodeJS 15, fixes [#732](#732) ([4548c11](4548c11))
Copy link

🎉 This issue has been resolved in version 10.16.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

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