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

Fix incompatibility with perf_hooks accross nodejs versions #515

Merged
merged 5 commits into from
Dec 31, 2022

Conversation

Fadoli
Copy link
Contributor

@Fadoli Fadoli commented Aug 31, 2022

When using rollup to build an all-in-one esm module, rollup fails to properly handle require in try{}catch{} code.

It causes perf_hooks to not be defined ( that would happens if perf_hooks isn't implemented in current nodejs version) and breaks code accessing perf_hooks constants fields :

perf_hooks.constants.NODE_PERFORMANCE_GC_MAJOR

It'd be nice to either require it (and as such require recent nodejs version) or make sure the codebase is compatible when it is not present.

This change is for enabling perf_hooks to not exists. It would also be possible to make it a hard requirement.

Note : perf_hooks.constants is defined only on nodejs version starting with the following :

  1. 13.9.0

  2. 12.17.0

Because of this I'm unsure the rest of the gc.js code will work correctly with nodejs >= 10 & <= 12.17

(Nodejs 10 requirement is from your package.json engines field)

Copy link
Collaborator

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks 👍

@SimenB
Copy link
Collaborator

SimenB commented Oct 27, 2022

We could also just make a major release dropping node 10, 12 and 17

@mireiaangles1109
Copy link

mireiaangles1109 commented Dec 13, 2022

Is there any update on this PR? We are facing the same issue and would be awesome to have it ready. @SimenB

Thanks!

@zbjornson
Copy link
Collaborator

I agree that it's reasonable to just drop 10, 12 and 17. v12 went EOL in April. @mireiaangles1109 can you use a current version of Node.js, or are you still running into issues with v14+?

@mireiaangles1109
Copy link

Thanks for your reply @zbjornson
We are trying to upgrade vite to the latest version (4.0.0) that uses rollup3. This is causing perf_hooks to not be defined as explained in this PR.

In our project we are using Node 16, so my guess is that the issues are coming with the rollup3 upgrade

Thanks!

@Fadoli
Copy link
Contributor Author

Fadoli commented Dec 14, 2022

hey @zbjornson , the actual issue is that rollup DOES NOT handle require in try {} catch {}, this causes perf_hooks to be undefined in the code, hence this modification is REQUIRED for it to work properly (By this I mean correctly fail due to missing perf_hooks but not break the whole code) in the current code.

The solution is either to get rid of the try {} catch {} arround the importation of perf_hooks, or to merge the changes I've got in this PR.

As of now your code is not compatible with building with rollup (meaning vite and all the other stuff that uses rollup as well)

@felipesantiago5555
Copy link

It would be awesome to get this merged, we are also facing similar issue with rollup.

@zbjornson
Copy link
Collaborator

It looks like Rollup has supported optional catch bindings since 2018. Do you need to set rollup's parser to the right ES version? rollup/rollup#2455

@gasparearmato6991
Copy link

We're having the same problem here using SvelteKit and Vite, it would great to merge this PR

@zbjornson
Copy link
Collaborator

zbjornson commented Dec 19, 2022

@gasparearmato6991 do you have Vite configured for the right ecmascript target? I have no idea if it will work to use Vite with this Node.js-only library, either.

@gasparearmato6991
Copy link

@zbjornson yes we're pointing the the right ES target, as mentioned before the problem here is that rollup does not handle the require in a try-catch block

@SimenB
Copy link
Collaborator

SimenB commented Dec 31, 2022

I think we can land this now and just revert when it's non-conditional in a future major version

@SimenB SimenB merged commit 7e2d65d into siimon:master Dec 31, 2022
@SimenB
Copy link
Collaborator

SimenB commented Dec 31, 2022

https://github.com/siimon/prom-client/releases/tag/v14.1.1

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.

6 participants