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: improve performance of getReport() #21

Merged
merged 3 commits into from
Mar 19, 2024

Conversation

styfle
Copy link
Contributor

@styfle styfle commented Mar 1, 2024

This will improve performance for newer versions of Node.js because this report doesn't need to access the network.

Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

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

Thank you for the PR Steven, I hadn't seen this new feature. Is it just me or is the API we have to use to set excludeNetwork somewhat awkward?

I've made a suggestion to move the /* istanbul ignore next */ exclusion up a level to the surrounding conditional, which I think should deal with the unit test failure.

I need to fix up the integration tests first as NodeSource have totally changed the way their repos work, plus a couple of the more rolling flavours of Linux such as Fedora 39 have upgraded their version of glibc within the last few months. Leave it with me.

UPDATE: Please can you rebase against main and the integration tests should pass again.

lib/process.js Show resolved Hide resolved
lib/process.js Outdated Show resolved Hide resolved
@styfle
Copy link
Contributor Author

styfle commented Mar 4, 2024

Is it just me or is the API we have to use to set excludeNetwork somewhat awkward?

Agreed. Its not just excludeNetwork thats awkward, but every Report configuration: https://nodejs.org/api/report.html#configuration

@styfle styfle force-pushed the improve-performance-for-get-report branch from fc742e5 to 48e937c Compare March 4, 2024 13:48
@styfle
Copy link
Contributor Author

styfle commented Mar 4, 2024

I'm also wondering if we should update CI to run against Node.js Nightly just to confirm this doesn't cause any problems (since this feature hasn't landed on the stable channel yet)

https://nodejs.org/download/nightly/v22.0.0-nightly20240304b34512e38e/

@lovell
Copy link
Owner

lovell commented Mar 4, 2024

Thanks for the updates. Let's wait until there's a proper, non-nightly (backported?) release of Node.js with the new flag before merging/releasing.

@lovell
Copy link
Owner

lovell commented Mar 19, 2024

I'm going to merge this now as the change in #22 means the use of getReport() should now mostly be confined to musl-based Linux.

There are no nightly Node.js builds for musl and the upstream excludeNetwork patch isn't planned for backport, so when Node.js 22 is released we can add it to the test matrix.

@lovell lovell merged commit 5e1482d into lovell:main Mar 19, 2024
34 checks passed
@lovell
Copy link
Owner

lovell commented Mar 19, 2024

Thanks again for the PR, this change was included in v2.0.3.

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.

2 participants