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

Failed to find should be an error, not a warning #192

Closed
call-a3 opened this issue Apr 7, 2016 · 10 comments
Closed

Failed to find should be an error, not a warning #192

call-a3 opened this issue Apr 7, 2016 · 10 comments
Milestone

Comments

@call-a3
Copy link

call-a3 commented Apr 7, 2016

Maybe there is a valid use-case for issuing a warning instead of an error that I don't know of. In that case I would suggest adding an option that can disable this "tolerancy" for unfound files.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 3, 2016

@MoOx Thoughts?

@MoOx
Copy link
Contributor

MoOx commented Nov 4, 2016

Might be a good idea to support that.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 16, 2016

@MoOx I can't see any valid case for warning. File not found is an Error. Will change this for v9 unless you disagree.

@RyanZim RyanZim added this to the v9.0.0 milestone Nov 16, 2016
@MoOx
Copy link
Contributor

MoOx commented Nov 17, 2016

From what I am seeing here, it's supposed to throw

throw new Error([

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 17, 2016

Yeah, but the next catch block turns it into a warning:

.catch(function(err) {
.

We need to add a catch block earlier for this error. Should we process.exit(1) for this error?

@MoOx
Copy link
Contributor

MoOx commented Nov 17, 2016

Han I didn't write this code, it's part of v8 :(
I would go for throwing again, using a setTimeout trick in the catch.

Library code should never ever do process.exit. Only binary should.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 17, 2016

I hadn't thought of setTimeout. I'm still not sure if that's a good idea; I can see people complaining about no way to handle the error. process.exit isn't a good idea either. Perhaps we should add a special case so that that catch block doesn't turn file not found into a warning. That way, we could just let the error trickle up the promise chain. Wouldn't be hard.

@MoOx
Copy link
Contributor

MoOx commented Nov 17, 2016

In the async world it's not easy. Maybe a option like onError, default to setTimeout that throw? This way by default it crash, and if people are not happy, they can catch the error.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 17, 2016

Not another option!!! Special-casing this error wouldn't be hard; I'll throw together a PR so you can see what I mean.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 18, 2016

Fixed by #247. Will be shipped in v9.

@RyanZim RyanZim closed this as completed Nov 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants