[4.x only] fix(router): do not finish bootstrap until all the routes are resolved by vsavkin · Pull Request #14155 · angular/angular

vsavkin · 2017-01-27T22:14:45Z

Updated 2.x branch #14327

vicb · 2017-02-03T19:06:14Z

should it be exported ?

You are right. It should be. As far as I know webworkers do not work with AoT, so it won't make a difference. I'll export it.

vicb · 2017-02-03T19:08:54Z

can we remove the reference to "experimental libs" here (I doubt we will update the comment later on) ?

vicb · 2017-02-03T19:10:54Z

any ->type ?

vicb · 2017-02-03T19:11:24Z

vicb · 2017-02-03T19:12:30Z

any -> type

vicb · 2017-02-03T19:14:25Z

should the default be null ? (what is the benefit of having an explicit default ?)

so we don't have to check for null and use polymorphism to handle this case. We use the same approach to handle other defaults in the router (e.g., defaultErrorHandler)

vicb · 2017-02-03T19:17:35Z

new Promise(res => res()) -> Promise.resolve(null) ?

vicb · 2017-02-03T19:18:41Z

vicb · 2017-02-03T19:26:40Z

do we have testsfor the return value ?

Every integration test checks this value since it is used by the navigation pipeline. If the value were incorrect, the navigation results would have been different.

makes sense

vicb · 2017-02-03T19:27:27Z

Thanks for the PR and sorry for the review delay.

Mostly nits, missing tests ?

vsavkin · 2017-02-06T23:29:34Z

Thanks for reviewing it! I addressed the comments.

I created bootstrap.spec.ts that tests the behaviour. What do you think should be tested separately?

vsavkin · 2017-02-06T23:29:54Z

Btw, I also have a version of this PR for the 2.x branch #14327

vicb · 2017-02-07T17:00:34Z

void vs null ?

void isn't an expression. I can use undefined, but I don't think we use undefined very much in the codebase, that's why I chose null.

vicb · 2017-02-07T17:02:51Z

remove "experimental"

mhevery · 2017-02-07T19:38:56Z

@vicb this breaks g3. Could you have a look?

vsavkin · 2017-02-07T22:37:39Z

Update: I cherry-picked the commit onto the 2.x branch. You can find it here: #14327

Fixes angular#12162

sync a 4.x change from angular#14155

sync a 4.x change from #14155

Fixes angular#12162 closes angular#14155

angular#14608) Fixes angular#12162 closes angular#14155

angular-automatic-lock-bot · 2019-09-10T21:22:22Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Jan 27, 2017

vsavkin force-pushed the router_boostrap_fix branch 8 times, most recently from 85329f7 to d7b75da Compare January 28, 2017 22:49

vicb added area: i18n Issues related to localization and internationalization action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 3, 2017

vicb reviewed Feb 3, 2017

View reviewed changes

vsavkin force-pushed the router_boostrap_fix branch from d7b75da to e9c420e Compare February 6, 2017 23:27

vicb changed the title ~~Router Bootstrap Fix~~ [4.x only] fix(router): do not finish bootstrap until all the routes are resolved Feb 7, 2017

vsavkin force-pushed the router_boostrap_fix branch from e9c420e to d95e1ec Compare February 7, 2017 16:18

vicb reviewed Feb 7, 2017

View reviewed changes

vsavkin force-pushed the router_boostrap_fix branch from d95e1ec to 9252c92 Compare February 7, 2017 17:21

vicb added the action: merge The PR is ready for merge by the caretaker label Feb 7, 2017

vicb added pr_state: LGTM (DEPRECATED USE GH REVIEWS) and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 7, 2017

vicb approved these changes Feb 7, 2017

View reviewed changes

fix(router): do not finish bootstrap until all the routes are resolved

f33f7b8

Fixes angular#12162

vsavkin force-pushed the router_boostrap_fix branch from 9252c92 to f33f7b8 Compare February 17, 2017 21:13

KyKyPy3 mentioned this pull request Feb 19, 2017

Router 3.4.8 tries to import missing token #14590

Closed

vicb removed the deprecated label: LGTM (DO NOT USE ANY MORE) label Feb 20, 2017

vicb added a commit to vicb/angular that referenced this pull request Feb 21, 2017

fix(router): improve robustness

bfefcb8

sync a 4.x change from angular#14155

vicb added a commit that referenced this pull request Feb 21, 2017

fix(router): improve robustness (#14602)

2a12346

sync a 4.x change from #14155

vicb mentioned this pull request Feb 21, 2017

fix(router): do not finish bootstrap until all the routes are resolved #14608

Merged

vicb pushed a commit to vicb/angular that referenced this pull request Feb 21, 2017

fix(router): do not finish bootstrap until all the routes are resolved

3bf4174

Fixes angular#12162 closes angular#14155

vicb closed this in 2a191ca Feb 21, 2017

vicb added a commit to vicb/angular that referenced this pull request Feb 21, 2017

fix(router): do not finish bootstrap until all the routes are resolved (

d550740

angular#14608) Fixes angular#12162 closes angular#14155

vicb mentioned this pull request Feb 21, 2017

fix(router): do not finish bootstrap until all the routes are resolve… #14625

Closed

asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017

fix(router): do not finish bootstrap until all the routes are resolved (

b473d12

angular#14608) Fixes angular#12162 closes angular#14155

juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017

fix(router): do not finish bootstrap until all the routes are resolved (

a678e99

angular#14608) Fixes angular#12162 closes angular#14155

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 10, 2019

Conversation

vsavkin commented Jan 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsavkin Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb commented Feb 3, 2017

Uh oh!

vsavkin commented Feb 6, 2017

Uh oh!

vsavkin commented Feb 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhevery commented Feb 7, 2017

Uh oh!

vsavkin commented Feb 7, 2017

Uh oh!

angular-automatic-lock-bot Bot commented Sep 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vsavkin commented Jan 27, 2017 •

edited

Loading

vsavkin Feb 6, 2017 •

edited

Loading