-
Notifications
You must be signed in to change notification settings - Fork 42
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
Using this plugin broke all external pointing links #63
Comments
You can add a
Will that solve your particular problem? |
Hmm, I suppose that could be done, but it seems like an unnecessary addition to be made to every external link, when it would be easy enough to assume that it shouldn't handle full urls. Then we wouldn't have to make markup changes to every external link in our code base in order to adopt the plugin. |
I think it's likely that we'll add support for automatically skipping external URLs, I just want to give it a little though first. |
I 👍 on adding this. If anyone want to implement this, please do |
One complication: if you have a catch-all route like this.route('not-found', { path: '/*path' }) then recognizeUrl will return true for any URL prefixed with the app's root. We have a number of applications mounted at different paths within the same origin. This shows up as a problem for the one app mounted at |
After implementing this plugin, we got major issues in production that all external pointing links directed to 404. This bug was not able to be reproduced locally.
It seems that the issue is that the plugin was trying to handle links that were using a normal href. The href apparently passed the checks to where it thinks it should handle it. It passed the url to the router, and got back the 404 route.
I think a reasonable fix would be to check the url in the href and look for a full url. For example, if the url starts with
http://www.blah.com/mypath
instead of/mypath
, it probably shouldn't handle the transition. For now we removed the plugin and won't be able to consider implementing it again unless we are confident this could be fixed.The text was updated successfully, but these errors were encountered: