-
Notifications
You must be signed in to change notification settings - Fork 30k
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
fs: refactor promises version of lchown and lchmod #20551
Conversation
What if we just did the check once during startup instead of each function call? That's what lib/fs.js currently does (except it actually doesn't assign the functions at all if symlinking isn't available). |
Updated to check at startup. |
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.
I personally like the previous variant better. Because of the trace. I don't think that moving !== undefined
check gives any measurable optimization and would prefer the trace to include the name of the function called.
If we want the check to be pre-calculated, I would prefer if the names of the exported functions were kept the same as they are now.
Taking into account that this changes the behavior in cases where the methods always throw, they are more likely to appear in logs, and not having the exact method name (the actual one that is not implemented) in the trace is inconvenient.
That's actually more of what I had in mind, just have the functions has variables and assign them to named functions in the |
It still is a little strange that we always have these functions available, which does not match the behavior of lib/fs.js, but that's for a separate PR I guess. |
I'm fine with either approach really. Just let me know which one to go with. I made the current changes in a second commit that can easily be discarded.
I think it's wrong (or at least odd) that the functions aren't there at all. From a user's perspective, the function not existing seems more like a bug in Node, compared to receiving a nice "not implemented" error message. |
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.
I'm fine with this but not sure it's critical
@ChALkeR I reverted to the original implementation that you signed off on. |
Check for platform support first to save a level of indentation. PR-URL: nodejs#20551 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Check for platform support first to save a level of indentation. PR-URL: #20551 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Check for platform support first to save a level of indentation. This is just a style thing, but it simplifies the code a bit IMO.
In case anyone points it out in the diff,
lchown()
is currently callingfchmod()
(incorrectly). I didn't update it because @ChALkeR has a fix for it in #20407.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes