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
Closed

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

wants to merge 6 commits into from

Conversation

daem0ndev
Copy link
Contributor

@daem0ndev daem0ndev 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

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@mhartington mhartington left a comment

Choose a reason for hiding this comment

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

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

@mhartington
Copy link
Contributor

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

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Mar 29, 2019

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

@rgolea
Copy link

rgolea 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
Copy link
Contributor

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
Copy link

rgolea commented Mar 29, 2019

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

@daem0ndev
Copy link
Contributor Author

@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-jw pushed a commit to kiku-jw/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
Labels
package: angular @ionic/angular package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants