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

error handling during downgraded component bootstrap #26024

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@FDIM
Copy link
Contributor

commented Sep 19, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

2 issues:

  • errors occurring in downgraded component constructor are unhandled and caught in zone.js
  • downgraded component is instantiated after scope is destroyed. In our project, since we are using lazy loading we are getting component factory not found errors.

Issue Number: #25613

What is the new behavior?

  • errors occurring in downgraded component constructor will be forwarded to angular.js $excetionHandler. We have missed some errors as they were not logged in production.
  • downgraded component will not be created if scope was destroyed before angular was bootstrapped

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@googlebot googlebot added the cla: yes label Sep 19, 2018

@FDIM FDIM changed the title errors related to downgraded component boostrap will be forwarded to $exceptionHandler errors related to downgraded component bootstrap will be forwarded to $exceptionHandler and component will no longer be created if scope is destroyed before injector is set Sep 19, 2018

@FDIM FDIM changed the title errors related to downgraded component bootstrap will be forwarded to $exceptionHandler and component will no longer be created if scope is destroyed before injector is set error handling during downgraded component bootstrap Sep 19, 2018

@gkalpak

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

Thx for the PR 👍 I haven't looked into the problems this is solving, but from a quick look it seems reasonable 😁
Can you add some tests demonstrating the problems being solved (and that they are indeed solved). I.e. we need some tests that would fail before and pass with the changes in the PR.

@maclun

maclun approved these changes Sep 20, 2018

Copy link

left a comment

The code change looks good. It works well in our setup and resolved the issue.

@FDIM

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2018

Hehe, I can try to reproduce them in some unit test, but it's not really easy as both of the fixes are for some edge cases.

  • $exceptionHandler is only about logging as some errors were not logged and swallowed by zone.js (#25613) (to clarify logged by console, but not in backend because you typically rely on $exceptionHandler)
  • checking for scope destroyed is an optimization in case some route resolver is taking a long time and user has already navigated away. For us this is an error because on route change we change 'current' lazy loaded module on ng-view (#17490) and thus component factory resolver fails to locate actual component

@FDIM FDIM force-pushed the FDIM:patch-1 branch from 7358cda to ce7e634 Sep 24, 2018

@FDIM

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

Ready for review. @gkalpak I've managed to cover both cases in specs :) And I am glad that you didn't approve as I forgot to add exceptionHandler as a dependency in initial commit :D

@gkalpak
Copy link
Member

left a comment

I left some minor comments, but otherwise LGTM. Thx, @FDIM 💯
(If you make further changes, please add them as new fixup commits 🙏 This makes them much easier to review.)

@Component({selector: 'ng2', template: 'test'})
class TestComponent implements OnInit {
constructor(@Inject('Not Existing') _notExisting: string) {}
ngOnInit() {}

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 26, 2018

Member

This is irrelevant. Please remove it.

This comment has been minimized.

Copy link
@FDIM

FDIM Sep 28, 2018

Author Contributor

done

imports: [BrowserModule]
})
class Ng2Module {
constructor() {}

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 26, 2018

Member

This is unnecessary. Please remove it.

@@ -609,6 +609,41 @@ withEachNg1Version(() => {
// Wait for the module to be bootstrapped.
setTimeout(() => expect($injectorFromNg2).toBe($injectorFromNg1));
}));

it('should forward errors to $exceptionHandler', (done) => {

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 26, 2018

Member

Why not use async() as in the other tests?

This comment has been minimized.

Copy link
@FDIM

FDIM Sep 28, 2018

Author Contributor

Error was caught by something in this case and not printed out by zone.js in the same way. Anyway, I've reverted the test to use async

@Component({selector: 'ng2', template: 'test'})
class TestComponent implements OnInit {
constructor() { testComponentCreated = true; }
ngOnInit() {}

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 26, 2018

Member

This is irrelevant. Please remove it.

This comment has been minimized.

Copy link
@FDIM

FDIM Sep 28, 2018

Author Contributor

done

constructor(@Inject('Not Existing') _notExisting: string) {}
ngOnInit() {}
}
@NgModule({

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 26, 2018

Member

A few empty lines here and there never hurt anyone 😛
(Here and below.)

This comment has been minimized.

Copy link
@FDIM

FDIM Sep 28, 2018

Author Contributor

done

imports: [BrowserModule]
})
class Ng2Module {
constructor() {}

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 26, 2018

Member

This is not needed. Please remove it.

This comment has been minimized.

Copy link
@FDIM

FDIM Sep 28, 2018

Author Contributor

done

.bootstrapModule(Ng2Module)
.then((module) => {
// simulate scope destruction
angular.element(element).injector !().get($ROOT_SCOPE).$destroy();

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 26, 2018

Member

Nit: You can use $injectorFromNg1.get() directly.

This comment has been minimized.

Copy link
@FDIM

FDIM Sep 28, 2018

Author Contributor

Done. This is much nicer :)

constructor() {}
ngDoBootstrap() {}
}
const element = html('<ng2 class="test"></ng2>');

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 26, 2018

Member

The class seems unnecessary.

This comment has been minimized.

Copy link
@FDIM

FDIM Sep 28, 2018

Author Contributor

removed

@@ -103,6 +104,12 @@ export function downgradeComponent(info: {
}

const doDowngrade = (injector: Injector) => {
// initial load can be async and the parent component may be destroyed before angular
// bootstrap e.g. slow resolve on some route and user navigation

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 26, 2018

Member

I would rephrase this to something like:

// Downgrading can happen asynchronously; e.g. an Angular module may need to be
// bootstrapped first. By the time we get here, the parent component may have been
// destroyed, in which case there is no need to instantiate the downgraded component.

This comment has been minimized.

Copy link
@FDIM

FDIM Sep 28, 2018

Author Contributor

done

FDIM added some commits Sep 28, 2018

@FDIM FDIM force-pushed the FDIM:patch-1 branch from ce7e634 to 23f2bf1 Sep 28, 2018

@FDIM

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

@gkalpak PR should be good now

@gkalpak
Copy link
Member

left a comment

LGTM 👍

I just think that downgrade_component_spec.ts might be a better place for these tests and we also need tests without downgradeModule(). Could you try moving the tests over to downgrade_component_spec.ts and add the same tests without downgradeModule()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.