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

Report errors when loading helpers #79

Merged
merged 1 commit into from Apr 4, 2016
Merged

Conversation

@jagoda
Copy link
Contributor

jagoda commented Mar 31, 2016

Fixes #73.

@jagoda jagoda added the feature label Mar 31, 2016
@jagoda jagoda added this to the 4.1.0 milestone Mar 31, 2016
@jagoda

This comment has been minimized.

Copy link
Contributor Author

jagoda commented Apr 1, 2016

@mtharrison @mugli this look like it meets your needs?

@mtharrison

This comment has been minimized.

Copy link
Member

mtharrison commented Apr 1, 2016

Having second thoughts about this, feels a bit wrong to me for a library to log to my stdout, even in warning situation. It could mess up whatever logging strategy I have in place. I wonder if there's a better way of doing this without throwing. Would it possible to use the hapi request.log()/server.log() here? Or at least use console.error() or console.warn() so it's sent to stderr.

Anyone else have a good idea?

@devinivy

This comment has been minimized.

Copy link
Member

devinivy commented Apr 1, 2016

I personally think sending it to stderr is sufficient, and I agree that sending it to stdout feels bad. I'm not totally convinced that hapi logging applies here very well. Hrm!

@jagoda

This comment has been minimized.

Copy link
Contributor Author

jagoda commented Apr 2, 2016

Thanks for the feedback! @mtharrison I considered server.log() but it seemed to run up against some of your earlier concerns that the warning may not be immediately apparent in the default case. This is because logging would have to be explicitly configured on the server (either via a plugin or the debug option) in order to see the output. Also, since this issue seems to skew toward the development cycle I don't think that a whole lot is lost by not sending the warning to server.log(). However, I had not considered how stdout could mess up other logging strategies. console.warn() seems reasonable to me. I will update accordingly if nobody can think of any final objections.

Fixes #73.
@jagoda jagoda force-pushed the jagoda:feat-helper-errors branch from 81091f5 to fa2e90a Apr 2, 2016
@jagoda jagoda merged commit ff9be92 into hapijs:master Apr 4, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jagoda jagoda deleted the jagoda:feat-helper-errors branch Apr 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.