-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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: throw fs.access errors in JS #17160
Conversation
The plan is to first migrate the fs errors into JS land, then migrate them to cc @jasnell |
588a6ad
to
4d0a61c
Compare
Test failed on Windows because
|
4d0a61c
to
f8bc6f9
Compare
CI looks green. |
This should be semver-major due to the error code changes of "path" type check cc @nodejs/tsc although we already have two TSC approvals |
|
||
let message = `${ctx.code}: ${ctx.message}, ${ctx.syscall}`; | ||
if (ctx.path) { | ||
const path = stringFromPath(ctx.path); |
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.
Also @fhinkel does this still LGTY with the stringFromPath
changes for fixing 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.
Still LGTM
src/node_file.cc
Outdated
if (!args[1]->IsInt32()) | ||
return TYPE_ERROR("mode must be an integer"); | ||
Local<Context> context = env->context(); | ||
CHECK(args.Length() >= 2 && args[1]->IsInt32()); |
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: Could we change this to two Check
s? If one of them fails it's easier to see what fails.
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.
@fhinkel Of course!
- Migrate the type check of path to ERR_INVALID_ARG_TYPE - Add template counterparts of ASYNC_CALL, ASYNC_DEST_CALL, SYNC_CALL, SYNC_DEST_CALL - Port StringFromPath and UVException to JavaScript - Migrate the access binding to collect the error context in C++, then throw the error in JS
f8bc6f9
to
51d8dac
Compare
Split the checks as suggested by @fhinkel . CI: https://ci.nodejs.org/job/node-test-pull-request/11701/ |
- Migrate the type check of path to ERR_INVALID_ARG_TYPE - Add template counterparts of ASYNC_CALL, ASYNC_DEST_CALL, SYNC_CALL, SYNC_DEST_CALL - Port StringFromPath and UVException to JavaScript - Migrate the access binding to collect the error context in C++, then throw the error in JS PR-URL: #17160 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Landed in 07d3409, thanks! |
SYNC_CALL, SYNC_DEST_CALL
then throw the error in JS
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
fs, errors