-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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: adjust typecheck for type
in fs.symlink()
#49741
fs: adjust typecheck for type
in fs.symlink()
#49741
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Ping @nodejs/TSC for reviews since this is semver-major. |
switch (type) { | ||
case undefined: | ||
case null: | ||
case 'file': | ||
return 0; | ||
case 'dir': | ||
return UV_FS_SYMLINK_DIR; | ||
case 'junction': | ||
return UV_FS_SYMLINK_JUNCTION; |
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.
switch (type) { | |
case undefined: | |
case null: | |
case 'file': | |
return 0; | |
case 'dir': | |
return UV_FS_SYMLINK_DIR; | |
case 'junction': | |
return UV_FS_SYMLINK_JUNCTION; | |
const linkTypes = { | |
undefined: 0, | |
null: 0, | |
file: 0, | |
dir: UV_FS_SYMLINK_DIR, | |
junction: UV_FS_SYMLINK_JUNCTION | |
}; | |
return linkTypes[type] || 0; |
The || 0
is a fallback in case the type is not found in the linkTypes object. If type is not one of the defined values in the linkTypes object, linkType will default to 0. This ensures that linkType always has a value and doesn't become undefined. It's a way to handle unexpected or unknown values for type.
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.
We intentionally don't fallback to file
. If an unknown type is passed, we must throw TypeError
, which is done by validateOneOf()
.
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
This comment was marked as outdated.
This comment was marked as outdated.
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.
Piggybacking on what others have said. I've noticed that this is a little older PR, however I've been spending time going thru them.
I notice in this one that it is a essential feature and I think that it needs to be merged as well.
LGTM?
FreeBSD CI is failing consistently
|
FreeBSD should be skipped. This needs a rebase to not be identified as Node.js 21. |
f05c863
to
53e1933
Compare
Throws `TypeError` instead of `Error` Enables autodetection on Windows if `type === undefined` Explicitly disallows unknown strings and non-string values
53e1933
to
5396244
Compare
Failed to start CI⚠ Something was pushed to the Pull Request branch since the last approving review. ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/8964951019 |
Landed in f202322 |
Throws `TypeError` instead of `Error` Enables autodetection on Windows if `type === undefined` Explicitly disallows unknown strings and non-string values PR-URL: nodejs#49741 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
Throws `TypeError` instead of `Error` Enables autodetection on Windows if `type === undefined` Explicitly disallows unknown strings and non-string values PR-URL: nodejs#49741 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
Throws `TypeError` instead of `Error` Enables autodetection on Windows if `type === undefined` Explicitly disallows unknown strings and non-string values PR-URL: nodejs#49741 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
Alternative to: #44641
TypeError
instead ofError
type === undefined
(as intended in win, fs: detect if symlink target is a directory #23724)