Skip to content
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

std/fs: Use original Error class for walk errors #3023

Closed
binyamin opened this issue Dec 19, 2022 · 6 comments · Fixed by #3054
Closed

std/fs: Use original Error class for walk errors #3023

binyamin opened this issue Dec 19, 2022 · 6 comments · Fixed by #3054

Comments

@binyamin
Copy link

Related to #1310

Summary

Whenever the "walk" function encounters an error, it transforms it using the code below. In the process, it uses a generic Error class as the constructor. This means that there's no easy way to identify the type of error.

https://github.com/denoland/deno_std/blob/7afe49284d0b5357eaf0f5668080cd26f13a1d8b/fs/walk.ts#L32-L42

Proposed Solution

Use new err.constructor() instead of new Error(), when instantiating the new error class.

@lino-levan
Copy link
Contributor

Could you elaborate on your use case here, I'm curious. In what situation would the error message not be good enough?

Not against this change, I think it makes sense(?) but I would appreciate further elaboration.

@binyamin
Copy link
Author

@lino-levan Sure.

First of all, a CLI tool might want custom error messages. It also might want to add additional context to the error. For example, the filepath, or the name of a plugin.

Secondly, if the walk function receives a path which doesn't exist, it throws. But If I check first that the path exists, I seem to be introducing a race condition. But I can't handle the error easily, because it's transformed to look like a generic error. In other cases, I would use instanceof or error.name.

The only way I've found to check if it's a "Not found" error, is to parse the stack-trace. Which is not ideal, and could get complicated.

@lino-levan
Copy link
Contributor

@iuioiua thoughts on this proposal? I would borderline push for a WalkError class with an err.cause defined as the error?

@lino-levan
Copy link
Contributor

After some further thinking, I think I like the idea of a WalkError with a cause defined as the actual error. I will open a PR for this to further the discussion.

@kt3k
Copy link
Member

kt3k commented Jan 17, 2023

@binyamin Does the suggested solution by @lino-levan support your use case? You can use the proposed fix from https://raw.githubusercontent.com/lino-levan/deno_std/fix-wrap-error/fs/walk.ts

import { walk } from "https://raw.githubusercontent.com/lino-levan/deno_std/fix-wrap-error/fs/walk.ts";

@binyamin
Copy link
Author

@kt3k Yes. It would be nice if WalkError had a path property, reused from the constructor arguments. Just an idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants