-
Notifications
You must be signed in to change notification settings - Fork 638
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(node/fs): chmod function throws unnecessary TypeError on Windows #3168
Conversation
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.
Mostly looks good to me. Left one comment
} catch (error) { | ||
// TODO(PolarETech): Errors should not be ignored when Deno.chmod is supported on Windows. | ||
// https://github.com/denoland/deno_std/issues/2916 | ||
if (Deno.build.os === "windows") { |
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.
nit: How about branching by this condition before doing try block?
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.
Then how about the following?
path = getValidatedPath(path).toString();
if (Deno.build.os === "windows" && mode === null) {
mode = 0;
}
mode = parseFileMode(mode, "mode");
Deno.chmod(pathModule.toNamespacedPath(path), mode).catch((error) => {
I realized that we might need to consider the case where Deno.chmod
supports Windows in the future, and I assume that if Deno.chmod
works, then parseFileMode
must be run.
It could be the case that Deno.chmod
is supported on Windows but the code here is left as is.
To prepare for such a case, I thought parseFileMode
would run, and if an error occurred, setting mode
to a dummy value (0 would be safest) would solve the current issue.
However, the concern remains that if a situation arises in the future where Deno.chmod
supports Windows but Deno.FileInfo.mode
does not, the permissions will be set to 0 unexpectedly.
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.
Thanks for the explanation. Now the current code just makes sense to me.
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.
LGTM
Fixes: #2916
Part of: denoland/deno#17841
This PR fixes an error that occurs when an invalid value is unintentionally passed to the
mode
parameter of thechmod
andchmodSync
functions on Windows.This change will make
npm:fs-extra.copy()
work on Windows.NOTE: This change must be reverted if
Deno.chmod
andDeno.chmodSync
were supported on Windows.