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

Remove errors handler from core #48

Merged
merged 1 commit into from Oct 14, 2016
Merged

Conversation

@frenzzy
Copy link
Member

frenzzy commented Sep 27, 2016

To handle the 404, add such route to the end:

const routes = [
  // your routes here
  { path: '*', action() { return 'Page not found' } }
]

To handle any other errors, use try catch:

try {
  await UniversalRouter.resolve(routes, '/path')
} catch (error) {
  // handle an error
}
@koistya

This comment has been minimized.

Copy link
Member

koistya commented Sep 27, 2016

This will make the code that uses Universal Router more complex. Why would you want to do that? Handling routing errors by hand isn't fun :) Also, with this approach you will have to create two routes - one for 404 page and another one for 500 page :(

@frenzzy

This comment has been minimized.

Copy link
Member Author

frenzzy commented Sep 27, 2016

Pros:

  1. Better performance: do not call heavy try catch statement for each route.
  2. Better performance: do not search /error route during each resolve call.
  3. Less magic: explicitly specifying the error handler.
  4. Allows to hide hardcoded /error page from website.
  5. Allows to handle errors in different way on the client and server-side.
  6. Allows to use different handles/layouts for error and 404 pages (but can be the same).

Cons:

  1. Error handler should be added manually to both client and server-side code.
  2. Allows to use the same route (component) to display 404 and other errors.
@frenzzy frenzzy force-pushed the frenzzy:remove-error-route branch from c023d66 to 41d1e62 Oct 9, 2016
@frenzzy frenzzy force-pushed the frenzzy:remove-error-route branch from 41d1e62 to a3605ae Oct 10, 2016
@frenzzy frenzzy added the discussion label Oct 11, 2016
@everdimension

This comment has been minimized.

Copy link

everdimension commented Oct 11, 2016

@frenzzy

To handle the 404, add such route to the end:

Can't you do the same right now if you just don't set the /error route?

Better performance: do not call heavy try catch statement for each route

But you're doing pretty much the same try catch in your example:

try { await UniversalRouter.resolve(routes, '/path') } catch (error) { // handle an error }

@frenzzy

This comment has been minimized.

Copy link
Member Author

frenzzy commented Oct 12, 2016

Can't you do the same right now if you just don't set the /error route?

Yes, we can.

But you're doing pretty much the same try catch in your example:

try {
  await UniversalRouter.resolve(routes, '/path')
} catch (error) {
  // handle an error
}

Yes, but not for each route separately.

One more example:

UniversalRouter.resolve(routes, '/path')
  .then(render)
  .catch(error => render(<ErrorPage error={error} />))
To handle the 404, add such route to the end:
```js
const routes = [
  // your routes here
  { path: '*', action() { return 'Page not found' } }
]
```

To handle any other errors, use try catch:
```js
try {
  await UniversalRouter.resolve(routes, '/path')
} catch (error) {
  // handle an error
}
```
@frenzzy frenzzy force-pushed the frenzzy:remove-error-route branch from a3605ae to 48b9991 Oct 12, 2016
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 12, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 48b9991 on frenzzy:remove-error-route into c947d00 on kriasoft:master.

@koistya koistya merged commit 4f69c62 into kriasoft:master Oct 14, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@frenzzy frenzzy deleted the frenzzy:remove-error-route branch Oct 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.