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.

Sign up for GitHub

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

Jump to bottom

fix(router): rethrow exceptions #2528

Closed
wants to merge 1 commit into from
Closed

fix(router): rethrow exceptions #2528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion modules/angular2/src/router/router.ts
View file
Open in desktop
Expand Up @@ -128,7 +128,10 @@ export class Router {
ObservableWrapper.callNext(this._subject, matchedInstruction.accumulatedUrl);
});

PromiseWrapper.catchError(result, (_) => this._finishNavigating());
PromiseWrapper.catchError(result, (err) => {
this._finishNavigating();
return err;
Copy link
Contributor

Choose a reason for hiding this comment

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

noop ? (the return value is not used)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same as .catch – so the error will be propagated to anyone consuming this promise.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are 2 anti-patterns here:

  1. Promise should usually be chained

Current Code

commitP = this.commit(matchedInstruction);
returnedP = commitP.then(...);
ignoredP = returnedP.catchError(...);

return returnedP;

It should be

commitP = this.commit(matchedInstruction);
returnedP = commitP.then(thenFn);
returnedP = returnP.catchError(catchFn);

return returnedP;

// or or more concise

returnedP = PromiseWrapper.then(thenFn, catchFn);

Note than the concise way is a slightly different behavior: if an exception is thrown is the thenFn, the catchFn is not called. If you want to catchFn to also handle errors in the thenFn, you should go for the first code.

  1. Behavior of catchFn

From an Promise reject function, you can either:

  • Recover,
  • Handle / Propagate the error

p = rejectedP.catch(e => return value) this is "recovery", p will be a Promise that resolves to value (it is not a rejected Promise.

p = rejected.catch(e => throw new Error()) equivalent to p = rejected.catch(e => Promise.reject(new Error())), p will be a rejected Promise - the first form is probably more usual but sometimes creates issue with TS which expects a return value for the callback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 See #2565

});

return result;
});
Expand Down
30 changes: 29 additions & 1 deletion modules/angular2/test/router/router_integration_spec.ts
View file
Open in desktop
Expand Up @@ -21,6 +21,8 @@ import {routerInjectables, Router} from 'angular2/router';
import {RouterOutlet} from 'angular2/src/router/router_outlet';
import {SpyLocation} from 'angular2/src/mock/location_mock';
import {Location} from 'angular2/src/router/location';
import {PromiseWrapper} from 'angular2/src/facade/async';
import {BaseException} from 'angular2/src/facade/lang';

export function main() {
describe('router injectables', () => {
Expand All @@ -47,6 +49,19 @@ export function main() {
});
}));

it('should rethrow exceptions from component constructors',
Copy link
Contributor

Choose a reason for hiding this comment

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

@btford does this test fail without the change in router.ts ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

Choose a reason for hiding this comment

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

nope (tested this time)

inject([AsyncTestCompleter], (async) => {
bootstrap(BrokenAppCmp, testBindings)
.then((applicationRef) => {
var router = applicationRef.hostComponent.router;
PromiseWrapper.catchError(router.navigate('/cause-error'), (error) => {
expect(el).toHaveText('outer { oh no }');
expect(error.message).toBe('oops!');
async.done();
});
});
}));

// TODO: add a test in which the child component has bindings
});
}
Expand All @@ -57,11 +72,24 @@ export function main() {
class HelloCmp {
}


@Component({selector: 'app-cmp'})
@View({template: "outer { <router-outlet></router-outlet> }", directives: [RouterOutlet]})
@RouteConfig([{path: '/', component: HelloCmp}])
class AppCmp {
router: Router;
constructor(router: Router) { this.router = router; }
}

@Component({selector: 'oops-cmp'})
@View({template: "oh no"})
class BrokenCmp {
constructor() { throw new BaseException('oops!'); }
}

@Component({selector: 'app-cmp'})
@View({template: "outer { <router-outlet></router-outlet> }", directives: [RouterOutlet]})
@RouteConfig([{path: '/cause-error', component: BrokenCmp}])
class BrokenAppCmp {
router: Router;
constructor(router: Router) { this.router = router; }
}