-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
lib: cache process.binding(…)
wrappers
#38132
Conversation
if (!result) { | ||
result = factory(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!result) { | |
result = factory(); | |
} | |
result ||= factory(); |
but do we want falsy or nullish?
if (!result) { | |
result = factory(); | |
} | |
result ??= factory(); |
There was a problem hiding this comment.
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()
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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();
?
There was a problem hiding this comment.
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?
Could we avoid the |
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. |
b2c15cd
to
b57a517
Compare
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. |
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. |
Currently,
process.binding("util") === process.binding("util")
istrue
, this ensures that with #37819,process.binding("util") === process.binding("util")
remainstrue
.Refs: #37819