Skip to content

Fix Link component conflicting with Angular router #4247

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix Link component conflicting with Angular router #4247

wants to merge 1 commit into from

Conversation

kujma10-zz
Copy link

@kujma10-zz kujma10-zz commented Dec 5, 2016 •
edited
Loading

This allows to create <a> tag without an href property.
The necessity of noHref property comes from the fact that angular router is in conflict with react-router's Link component. To be more precise, Angular is listening to all click events and if the click is on an tag with an href that it recognizes as one of its routes and it calls preventDefault() on the event. The Link implementation checks for default being prevented and does nothing if it has.

Fix Link component conflicting with Angular router
b9efc08 
This allows to create <a> tag without "href" property.
The necessity of this property comes from the fact that angular router is in conflict with react-router's Link component. To be more precise, Angular is [listening to all click events][1] and if the click is on an <a> tag with an href that [it recognizes as one of its own routes][2] then [it calls preventDefault() on the event][3]. The Link implementation [checks for default being prevented][4] and does nothing if it has.

[1]: https://github.com/angular/angular/blob/cf269d9ff43b913dbac4ba0ed6932c708ae58512/modules/angular1_router/src/ng_route_shim.js#L285
[2]: https://github.com/angular/angular/blob/cf269d9ff43b913dbac4ba0ed6932c708ae58512/modules/angular1_router/src/ng_route_shim.js#L294
[3]: https://github.com/angular/angular/blob/cf269d9ff43b913dbac4ba0ed6932c708ae58512/modules/angular1_router/src/ng_route_shim.js#L296
[4]: https://github.com/ReactTraining/react-router/blob/master/modules/Link.js#L78
@timdorr
Copy link
Member

timdorr commented Dec 5, 2016

That click listener in Angular only listens to elements it controls. So, you shouldn't be ng-app'ing something that includes React code. Isolate those things and the problem goes away.

Also, we don't need to make affordances for outside scripts like this. If anything, we should check for the emptiness of toLocation, rather than adding another prop to the component.

@timdorr timdorr closed this Dec 5, 2016
@deiwin
Copy link

deiwin commented Dec 5, 2016

I helped @kujma10 put this together and think you should reconsider including this change.

That click listener in Angular only listens to elements it controls. So, you shouldn't be ng-app'ing something that includes React code. Isolate those things and the problem goes away.

We're building a React application that is embedded into all kinds of different pages that we have no control over. This includes many pages built with Angular, whose documentation suggests adding the ng-app to the <html> or <body> tags:

The ngApp directive designates the root element of the application and is typically placed near the root element of the page - e.g. on the <body> or <html> tags.

This means we're stuck with this Angular code listening to our elements. (Yes Shadow DOM could help here, but unfortunately browser support for it is still too weak for this to really be an option for us.) And I don't think we're alone in working in this kind of setups. See similar concerns raised in this issue, for example. Further, isn't the option of avoiding conflicts with other frameworks the main reason for why MemoryHistory support was added? Yes, we could solve it on our side by essentially not using Link at all and implementing our own version of it, but I believe it could benefit others as well.

Also, we don't need to make affordances for outside scripts like this. If anything, we should check for the emptiness of toLocation, rather than adding another prop to the component.

Unfortunately that's not the case. We still need to pass a proper to prop for the handleClick function to work and actually push the new route. I can see how specifying the href can be useful as a fallback when used together with browser/hash history, but frankly, it's useless and even potentially harmful when used together with memory history - it can clash with other frameworks, as shown in this case with Angular, and if handleClick fails to call preventDefault() for any reason (early return/error) the browser could navigate the user to a nonexistent and undesired URL.

@kujma10-zz
Copy link
Author

@timdorr can you please reconsider this Pull Request?

@kujma10-zz kujma10-zz mentioned this pull request Apr 25, 2017
Merged
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kujma10-zz @timdorr @deiwin