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

lib: cache process.binding(…) wrappers #38132

Closed

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Apr 7, 2021

Currently, process.binding("util") === process.binding("util") is true, this ensures that with #37819, process.binding("util") === process.binding("util") remains true.


Refs: #37819

Comment on lines 44 to 46
if (!result) {
result = factory();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!result) {
result = factory();
}
result ||= factory();

but do we want falsy or nullish?

Suggested change
if (!result) {
result = factory();
}
result ??= factory();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arguably, it can just be return result ??= factory().

Copy link
Contributor

Choose a reason for hiding this comment

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

Arguably, it can just be return result ??= factory().

I don't know if we have a linter rule enforcing that, but I think we tend to avoid mixing assignments and return statements in the code base, to improve readability.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are a couple of assignments in return statements:

$ git grep "return .* = " lib

Since this is a smallish statement, should we prefer return result ??= factory();?

Copy link
Member

Choose a reason for hiding this comment

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

Arguably, it can just be return result ??= factory().

I don't know if we have a linter rule enforcing that, but I think we tend to avoid mixing assignments and return statements in the code base, to improve readability.

Why would that improve readability?

@aduh95
Copy link
Contributor

aduh95 commented Apr 7, 2021

Could we avoid the %Object.fromEntries% call in factories and the followup%Object.entries% call in the module.exports? Or refactor factories be an array maybe?

@aduh95 aduh95 added dont-land-on-v10.x process Issues and PRs related to the process subsystem. labels Apr 7, 2021
@jasnell jasnell requested a review from addaleax April 7, 2021 13:53
@addaleax
Copy link
Member

addaleax commented Apr 7, 2021

Currently, process.binding("util") === process.binding("util") is true, this ensures that with #37819, process.binding("util") === process.binding("util") remains true.

To be frank – Why do we care? This is not something that userland code would be expected to rely on, and the point here is to reduce maintenance overhead by putting code in a separate file and not caring about that beyond minimal maintenance. Yes, it’s a bit slower to access the bindings this way, but that’s just fine too.

@github-actions
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

Copy link
Contributor

Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

@github-actions github-actions bot closed this Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants