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

fix(router-outlet): support relative router links #17888

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
4 participants
@daem0ndev
Copy link
Contributor

commented Mar 26, 2019

Short description of what this resolves:

This PR resolves an issue where relative router links break after a forward and back navigation to a component that has already been created in the ion router outlet stack.

Changes proposed in this pull request:

  • Create activated route proxy for each component instance activated in the outlet
  • Update the underlying observables on the proxy so consumers need not be aware of implementation details

Ionic Version:
4.x

Fixes: #16534, #16736, #16954

daem0ndev and others added some commits Mar 26, 2019

@liamdebeasi
Copy link
Member

left a comment

So I did some testing and took a brief look over the code. The issue is fixed and the code looks good! Only thing I'd suggest is maybe adding more comments setupProxyObservables.
I am going to work on some automated tests today/tomorrow.

liamdebeasi added some commits Mar 28, 2019

@mhartington
Copy link
Member

left a comment

Only comment (more of code style) is to not format with prettier. But everything else looks good.

@mhartington

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

Merged! Thanks @daem0ndev for all your work on this and doing the investigation.

@liamdebeasi

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

This will be in the next release of Ionic. I am not certain of the version number yet.

@rgolea

This comment has been minimized.

Copy link

commented Mar 29, 2019

Great! Some minor breaking changes I've got as I just tested it, is that if you were using something like the example below, you will get route.params as undefined and it will get instanciated afterwards. You will have to translate that code inside ngOnInit (no biggie, just to write it down in breaking changes):

@Component({
  ....
})
export class MyComponent(){
    public id$ = this.route.params.pipe(map(params => params.id));

    constructor(
         private readonly route: ActivatedRoute
    ){}
}

This will have to be translated to this

@Component({
  ....
})
export class MyComponent implements OnInit(){
    public id$:Observable<string> = null;

    constructor(
         private readonly route: ActivatedRoute
    ){}

    ngOnInit(){
          this.id$ =  this.route.params.pipe(map(params => params.id));
    }
}
@mhartington

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

I wouldn't consider this a breaking change actually, as the example above is really an anti-pattern/not-recommended. We can make a note, but it should have been avoided to begin with.

@rgolea

This comment has been minimized.

Copy link

commented Mar 29, 2019

@mhartington sure! Just a note will go a long way. Thank you for the fix @daem0ndev!

@daem0ndev

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

@rgolea @mhartington I created this PR to resolve the breaking change. I do agree its breaking since in a vanilla angular app, you can bind to the observables prior to ngOnInit. #17914

Kiku-git added a commit to Kiku-git/ionic that referenced this pull request May 16, 2019

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.