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

#45 Pass url while creating 404 error message #46

Closed
wants to merge 2 commits into from

Conversation

bhargavigundaa
Copy link

Lets say I've received an 404 error in Universal router . And I'm trying to debug the Url that caused the error, I cant see the url as while creating the error we are not passing the URL

Lets say I've received an 404 error in Universal router . And I'm trying to debug the Url that caused the error, I cant see the url as while creating the error we are not passing the URL
@sap9433
Copy link

sap9433 commented Sep 27, 2016

So we have been using universal router at production with https://github.com/kriasoft/react-starter-kit. In error page we are trying to log the error URL , but saw it's never being passed . With this change that issue is solved.

I tested it . can you kindly merge.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.406% when pulling 145b7f7 on bhargavigundaa:patch-1 into 7a58b09 on kriasoft:master.

@bhargavigundaa
Copy link
Author

  1. Lets say user have reached this page - https://github.com/kriasoft/react-starter-kit/blob/master/src/routes/error/ErrorPage.js. He wont know which URL caused this .

  2. A url call failed at server side and passed in client side . Now user sees error page coming from backend , and after a few mili-seconds client side rendered page shows up . Now users are complaining about this issue in prod and you dont know what's causing this . Solution is add following code at Line number 15 in the error above page. That does the server logging for you, but it's not much of help , as it's not printing the URL.

Copy link
Collaborator

@langpavel langpavel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be backward compatible, more helpfull and safer:

  if (result === undefined && errorRoute) {
    const error = new Error('Not found');
    error.status = 404;
    error.context = { ...context };
    context.error = error;
    return await errorRoute.action(context, {});
  }

And please, add test if possible

@sap9433
Copy link

sap9433 commented Oct 7, 2016

not sure why it's backwork compatible and our solution wasn't.

all i know is ours was tested and live on one of india's largest ecommerce
store. And we are facing problem as its taking time to merge even the
simplest PR

On 7 Oct 2016 4:34 a.m., "Pavel Lang" notifications@github.com wrote:

@langpavel requested changes on this pull request.

This will be backward compatible, more helpfull and safer:

if (result === undefined && errorRoute) {
const error = new Error('Not found');
error.status = 404;
error.context = { ...context };
context.error = error;
return await errorRoute.action(context, {});
}

And please, add test if possible


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#46 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEI_pvzF2_aMZ6745OvB0KXztdIu_8Nmks5qxX5qgaJpZM4KHqPE
.

@langpavel
Copy link
Collaborator

I think this error should tightly follow HTTP status codes. Additional info should not be part of message

@frenzzy
Copy link
Member

frenzzy commented Oct 20, 2016

This is out of date because /error route has been removed from the core in #48
Also since v3.0.0 you can use error.context.url

@frenzzy frenzzy closed this Oct 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants