-
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
doc: deprecate non-string type
in fs.symlink()
#44641
doc: deprecate non-string type
in fs.symlink()
#44641
Conversation
Disallows any `type` except for `'dir'`, `'file'`, `'junction'`, or `undefined` in `fs.symlink()`, `fs.symlinkSync()`, and `fsPromises.symlink()`
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.
Why not leave this validation at the FS level, or libuv? Doing it in Node.js seems weird to me, and potentially bad for forward compatibility.
/cc @nodejs/fs |
libuv receives flags ( |
Oh so we don't need a deprecation, using an invalid string is already throwing. |
@LiviaMedeiros is #44641 (comment) correct or am I missing something? |
If I remember correctly, it's true for invalid strings; but any parameter with |
The documentation already indicates it should be a string, I don't think we need a deprecation for that. It sounds like we can tighten up the check as a semver major directly, unless there's a massive ecosystem breakage of doing so. |
Disallows any
type
except for'dir'
,'file'
,'junction'
, orundefined
infs.symlink()
,fs.symlinkSync()
, andfsPromises.symlink()
The implementation would be unconditional validating with
validateOneOf(type, 'type', ['dir', 'file', 'junction', undefined])
, i.e. throwing an error instead of defaulting to autodetection (or instead of ignoring on non-windows platforms).Refs: #42894 (comment)
Not sure if it's worth the potential breakage. On a long run, replacing it with an object parameter (for example,
{ type: 'file', force: true, relative: false }
) to allow additional options might make more sense.