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

Router upgrade does not work when initial state is Angular #18329

Closed
jbedard opened this issue Jul 25, 2017 · 9 comments
Closed

Router upgrade does not work when initial state is Angular #18329

jbedard opened this issue Jul 25, 2017 · 9 comments
Labels
area: router area: upgrade Issues related to AngularJS → Angular upgrade APIs effort1: hours freq2: medium needs reproduction This issue needs a reproduction in order for the team to investigate further type: bug/fix
Milestone

Comments

@jbedard
Copy link
Contributor

jbedard commented Jul 25, 2017

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior

Very similar setup to #14081, but instead of using RouterUpgradeInitializer (which doesn't work) you can manually invoke setUpLocationSync() directly after UpgradeModule#bootstrap. This correctly sets up the location-syncing, but does not pickup the initial state because the initial $locationChangeStart event from AngualrJS has already fired during the UpgradeModule#bootstrap.

Manually invoking Router.navigateByUrl an extra time after setUpLocationSync() fixes it.

Currently I've replaced setUpLocationSync with a modified version:

//Copied from @angular/router/upgrade `setUpLocationSync`, plus an initial `navigateByUrl`
function setUpLocationSync(upgrade: UpgradeModule) {
    const router = upgrade.injector.get(Router);
    const url = document.createElement("a");
    function hrefToRouter(href) {
        url.href = href;
        router.navigateByUrl(url.pathname + url.search + url.hash);
    }

    upgrade.$injector.get("$rootScope").$on("$locationChangeStart", function(_, next, __) {
        hrefToRouter(next);
    });

    //Also do an initial `navigateByUrl` which is missing from the @angular/router/upgrade version
    hrefToRouter(upgrade.$injector.get("$location").url());
}

Expected behavior

Loading an Angular route on initial page load should work

Minimal reproduction of the problem with instructions

#14081 except invoke setUpLocationSync(this.upgrade); directly after the UpgradeModule#bootstrap call and drop RouterUpgradeInitializer

@gkalpak
Copy link
Member

gkalpak commented Dec 15, 2017

To avoid the sometimes unnecessary extra call, I think we could set up a listener for $locationChangeStart early on and save it so it can be accessed by setUpLocationSync() later.

@ngbot ngbot bot added this to the Backlog milestone Jan 23, 2018
@gkalpak gkalpak added area: upgrade Issues related to AngularJS → Angular upgrade APIs and removed area: upgrade Issues related to AngularJS → Angular upgrade APIs comp: upgrade/static labels Mar 13, 2019
@aaronfrost
Copy link
Contributor

Any work arounds on this? @gkalpak @jbedard

@gkalpak
Copy link
Member

gkalpak commented Jul 30, 2019

@jbedard included the workaround in his original comment 😁

@gkalpak
Copy link
Member

gkalpak commented May 27, 2020

This issue is quite old and I can't tell whether it is still relevant. I tried to reproduce it in this StackBlitz project, but couldn't. However, there are numerous ways to configure a dual router setup, so it might be something specific to my setup that avoids the problem.

@jbedard, @aaronfrost: If you are still running into this issue, could you share a minimal reproduction, so we can confirm whether this is still an issue and investigate further?

@gkalpak gkalpak added needs reproduction This issue needs a reproduction in order for the team to investigate further triage #1 labels May 27, 2020
@jbedard
Copy link
Contributor Author

jbedard commented Jun 1, 2020

Looks like I removed the workaround for this just a couple months after filing this issue. It was after a couple upgrade/router related changes, but I'm not really sure which if any of those changes fixed it, or maybe it was fixed in an angular upgrade 🤷

One change I did right before dropping the workaround was changing what's in ngDoBootstrap from

this.upgrade.bootstrap(document.body, [appModule.name], {strictDi: true});
setUpLocationSync(this.upgrade);

to

appModule.run(() => setUpLocationSync(this.upgrade));
this.upgrade.bootstrap(document.body, [appModule.name], {strictDi: true});

(which was done for some reason related to protractor patching something, and doing setUpLocationSync synchronously here broke that yet doing it in appModule.run fixed it?)

Looks like that .run change broke loading the app on an Angular route (essentially another version of this issue). I then deleted my custom setUpLocationSync which fixed that and didn't reproduce this issue? 🤷

Our routing/upgrade/a1<=>2 logic has completely changed since then so I don't think it's worth me even trying to reproduce it atm

@jbedard
Copy link
Contributor Author

jbedard commented Jun 1, 2020

After looking at your StackBlitz though... one big difference was that I had the upgrade.bootstrap within the AppModule.ngDoBootstrap, I also didn't have a UrlHandlingStrategy at the time (but did add one just like yours shortly before removing the workaround for this issue).

Your StackBlitz also has no call to setUpLocationSync at all though?

@gkalpak
Copy link
Member

gkalpak commented Jun 1, 2020

Yeah, it doesn't 😁 TBH, I am not sure what issue exactly I was trying to reproduce 😇

@atscott
Copy link
Contributor

atscott commented Jul 10, 2020

Since we can't seem to figure out exactly how to reproduce the issue here or if it's fixed or not, I'm going to close this.

If the problem still exists in your application, please open a new issue and follow the instructions in the issue template that include info on how to create a reproduction using our template.

@atscott atscott closed this as completed Jul 10, 2020
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: router area: upgrade Issues related to AngularJS → Angular upgrade APIs effort1: hours freq2: medium needs reproduction This issue needs a reproduction in order for the team to investigate further type: bug/fix
Projects
None yet
Development

No branches or pull requests

6 participants