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

Provide custom onError handler for http-proxy-middleware #502

Merged
merged 3 commits into from
Sep 3, 2016

Conversation

cloudmu
Copy link
Contributor

@cloudmu cloudmu commented Aug 26, 2016

This will change the http-proxy-middle logLevel option from 'silent' to 'error' in development mode.

Errors are bound to occur when proxying API requests to an API server during development, for various reasons such as forgetting to start the API server or using an incorrect port. With 'silent' option, no error message will be shown in the command line console at all, which can be confusing. With 'error' option, the console will show a very concise and informative error message, which helps to quickly pinpoint to the issues.

For example, following is an example of the error message (caused by not starting the API server) in the command line console:

proxy-error

@gaearon
Copy link
Contributor

gaearon commented Aug 27, 2016

This error message looks very confusing and aggressive. Can we improve the error output of this Node module first, to be readable in common cases?

@cloudmu
Copy link
Contributor Author

cloudmu commented Aug 28, 2016

Personally I think the error message (as is) might still be better than being silent. It gives some hints, such as keyword "Proxy Error", standard POSIX error code, and the hostname and targetUri the proxy failed to access. However I totally understand the concern and CRA's principled approach of being beginner friendly.

I looked into how the error messages (with error codes such as ECONNREFUSED) is formulated:

  1. http-proxy-middleware (a.k.a. HPM) simply prints out the error code thrown from another lib http-proxy.
    See the logError impl here
    logger.error('[HPM] PROXY ERROR: %s. %s -> %s', err.code, hostname, targetUri);
  2. http-proxy in turn gets the error code thrown from Node.js. See Node Common System Errors
    Note it's great that for each error code, there is corresponding explanation in plain text (which would be great if CRA can refer to).
  3. To further trace back to the origin (not that we need to worry here, but just for completeness), the Common System Errors are ultimately generated from within the runtime environment at syscall level, See errno

So as @gaearon suggested, the straightforward approach may be to improve the error output in http-proxy-middleware (or http-proxy). The logError implementation can simply reference the NodeJS common System Error page (as a link), and/or print out the plain text explanations mapped to the common errors such as ECONNREFUSED, ECONNRESET, and ETIMEDOUT.

@ghost ghost added the CLA Signed label Aug 28, 2016
@geowarin
Copy link
Contributor

Errno is a cool npm package for the purpose of translating node's errors

@gaearon
Copy link
Contributor

gaearon commented Aug 28, 2016

So as @gaearon suggested, the straightforward approach may be to improve the error output in http-proxy-middleware (or http-proxy). The logError implementation can simply reference the NodeJS common System Error page (as a link), and/or print out the plain text explanations mapped to the common errors such as ECONNREFUSED, ECONNRESET, and ETIMEDOUT.

Errno is a cool npm package for the purpose of translating node's errors

Sounds good to me, would you like to send a PR to http-proxy-middleware?

@ghost ghost added the CLA Signed label Aug 28, 2016
@cloudmu
Copy link
Contributor Author

cloudmu commented Aug 28, 2016

Yes I can do a PR to http-proxy-middleware.
Unfortunately Errno would only provide a very simple description by default. E.g. "connection refused" for "ECONNREFUSED".

So we can either

  1. further customize the Errno description by using the text from Node Common System Error
    or
  2. reference the Node Common System Error as a html link directly in the console error log.

Any preference?

@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

We decided to update this to use onError, is that correct?

@cloudmu
Copy link
Contributor Author

cloudmu commented Sep 2, 2016

@gaearon Yes we decided to use onError. Just got around to implement it. Two options with slight differences.
1 No chalk color used:
option1

2 Cyan color for url and links:
option2

Any preference? I will check in code later on.

@cloudmu cloudmu changed the title Change http-proxy-middleware logLevel from silent to error Provide custom onError handler for http-proxy-middleware Sep 2, 2016
@ghost ghost added the CLA Signed label Sep 2, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

Let’s make it An error instead of Error, keep cyan, and additionally highlight the error type with red.

@cloudmu
Copy link
Contributor Author

cloudmu commented Sep 2, 2016

Like this?
image

@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

Hmm. It’s still a bit too easy to miss it.

Maybe try <red>Proxy error:</red> Could not proxy request ...?
I think worth trying a few different variations.

@cloudmu
Copy link
Contributor Author

cloudmu commented Sep 2, 2016

Like the suggestion. It reads better, and makes Proxy error more obvious.
image

To nitpick, we can keep error code plain, to make the console a little less busy.
image

Pick one :-)

@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

This looks good.

@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

Let’s do the second one. I would probably remove cyan from Node docs link and maybe add cyan to the error code instead. Also could try putting the whole second line into chalk.dim.

@cloudmu
Copy link
Contributor Author

cloudmu commented Sep 2, 2016

I will go with this for now.
image

chalk.dim works on Mac, but not on Windows 10 (in my testing).

@gaearon gaearon merged commit ec60307 into facebook:master Sep 3, 2016
@gaearon gaearon added this to the 1.0.0 milestone Sep 3, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 3, 2016

Beautiful work. Thanks.

@gaearon gaearon added this to the 0.4.1 milestone Sep 3, 2016
@gaearon gaearon removed this from the 1.0.0 milestone Sep 3, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 3, 2016

Out in 0.4.1!

stayradiated pushed a commit to stayradiated/create-react-app that referenced this pull request Sep 7, 2016
* Change http-proxy-middleware logLevel from silent to error

* provide onError handler for httpProxyMiddleware
feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
* Change http-proxy-middleware logLevel from silent to error

* provide onError handler for httpProxyMiddleware
@lileilei
Copy link

lileilei commented Dec 1, 2017

http-party/node-http-proxy#579 Look at this issue!

@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants