Skip to content

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

Merged
gaearon merged 3 commits into
react:masterfrom
cloudmu:master
Sep 3, 2016
Merged

Provide custom onError handler for http-proxy-middleware#502
gaearon merged 3 commits into
react:masterfrom
cloudmu:master

Conversation

@cloudmu

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

cloudmu commented Aug 28, 2016

Copy link
Copy Markdown
ContributorAuthor

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.

@ghostghost added the CLA Signed label Aug 28, 2016
@geowarin

Copy link
Copy Markdown
Contributor

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

@gaearon

Copy link
Copy Markdown
Contributor

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?

@ghostghost added the CLA Signed label Aug 28, 2016
@cloudmu

cloudmu commented Aug 28, 2016

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
Contributor

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

@cloudmu

Copy link
Copy Markdown
ContributorAuthor

@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.

@cloudmucloudmu changed the title Change http-proxy-middleware logLevel from silent to errorProvide custom onError handler for http-proxy-middlewareSep 2, 2016
@ghostghost added the CLA Signed label Sep 2, 2016
@gaearon

Copy link
Copy Markdown
Contributor

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

@cloudmu

Copy link
Copy Markdown
ContributorAuthor

Like this?
image

@gaearon

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
Contributor

This looks good.

@gaearon

gaearon commented Sep 2, 2016

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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 react:masterSep 3, 2016
@gaearongaearon added this to the 1.0.0 milestone Sep 3, 2016
@gaearon

Copy link
Copy Markdown
Contributor

Beautiful work. Thanks.

@gaearongaearon added this to the 0.4.1 milestone Sep 3, 2016
@gaearongaearon removed this from the 1.0.0 milestone Sep 3, 2016
@gaearon

Copy link
Copy Markdown
Contributor

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
Copy Markdown

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

@locklockBot locked and limited conversation to collaborators Jan 20, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@cloudmu@gaearon@geowarin@lileilei@facebook-github-bot