aurelia / router

A powerful client-side router.
MIT License
120 stars 115 forks source link

Ability to ignore a specific URL/link #428

Open fracz opened 7 years ago

fracz commented 7 years ago

I'm submitting a feature request / opening a discussion

I have a router configured with HTML5 push state enabled. When I try to click links that download static files such as:

<a href="/files/some-file.xlsx">Download</a>

the router complains:

ERROR [app-router] Error: Route not found: /files/some-file.xlsx

I have solved this by adding target="_top" to the link but I'm not sure if this is a correct solution.

<a href="/files/some-file.xlsx" target="_top">Download</a>

I have found that competition solves this by adding target="_self" to such links. It also ignores all the links starting with slash.

Maybe aurelia-router should implement similar behavior?

EisenbergEffect commented 7 years ago

@bryanrsmith What do you think about this?

davismj commented 7 years ago

@fracz What happens if you use target="_blank. That's probably what I would recommend anyway, so I'd be curious to know if it would work.

EisenbergEffect commented 7 years ago

Closing since no followup from OP in over 3 weeks.

benstevens48 commented 6 years ago

@EisenbergEffect I also think there should be some way of preventing the router from processing a link (i.e. <a href="...">...</a>).

In addition to the OP's example, this is needed in order to link from part of a site that uses Aurelia client-side routing to another part of a site which does not use client-side routing (and possibly does not use Aurelia).

Ignoring links with the attribute target="_self", or indeed any target attribute, seems to be a standard way of doing this (see Angular 1, page.js). An alternative would be to create a directive to do this, but I definitely think it should be built-in to Aurelia. Also, if not the case already, the router should also ignore links with the download attribute.

davismj commented 6 years ago

@benstevens48 @fracz Take a look at the feature request here: https://github.com/aurelia/router/issues/527

I think this is probably the most robust way to solve these kinds of problems. I have this on my radar for a 1.6.0 release of the router.

benstevens48 commented 6 years ago

Oh, yes, that also works, and is probably good enough, although it's not exactly the same. For example, you might still want to have a custom 404 not found page in your Aurelia app (so not ignore unmatched routes), whilst still specifically creating some links that should result in requests to the server.

davismj commented 6 years ago

Hm. Good, really good point, @benstevens48

@PWKad can I get your input here. What was your use case for #527 ? You had mentioned like a fall through to the web server, but I'm wondering if there isn't a more general answer?

What about this approach?

Add a fall through route config

{ route: 'safe/to/download/path/*', ignore: true }

Then we could pass through all unknown routes (not recommended):

config.mapUnknownRoutes(() => { ignore: true })

The advantage to this approach is that it would give a bit of flexibility on what would pass through, while preventing a careless developer from exposing sensitive files by accident.

benstevens48 commented 6 years ago

I haven't had a lot of time to think about this and I'm not very familiar with the router, but I'm going to reply now else I never will.

There is a difference between ignoring the route and fetching from the server. In the case that a link was clicked, ignoring the route would result in a server request as long as event.preventDefault() was not called (so this needs checking). In the case of calling router.navigate this would simply do nothing, presumably. If the mapUnknownRoutes function was provided with the requested url then it would be easy to manually trigger a request to the server using location.href = url, but it might also be nice to be able to return an ignore parameter as you suggested in case you would want to do something asynchronous first. Also I don't know whether this would break certain default behaviors like the download attribute.

Having the ignore parameter on other routes as well could also be useful - especially if there was the ability to run an arbitrary function when the route was matched (sorry if this is already possible, I don't have time to investigate now).

Going back to the original point of this post, I still feel like it's a bit inconvenient to have to manually add links to ignore to the router config rather than being able to specify this as part of the html A tag.

Also, just to add, I'm not that concerned about security aspects since I think sensitive files should always be protected server-side.

davismj commented 6 years ago

@EisenbergEffect Would also like your input.

davismj commented 6 years ago

@benstevens48 Does this work? Can you enhance this gist to show me what isn't working? https://gist.run/?id=d7c1b92146c250e726da904e8f08b417

benstevens48 commented 6 years ago

You need to set the pushState option to true. This might not be the best way to test it because you can't really see the URL in the address bar. Here is a forked gist - https://gist.run/?id=885010d294c6039d94db03e394b7fa59. The links download1 and test link 1 work, whereas the links download 2 and test link 2 do not work. In my opinion a link with the download attribute should never be handled by the router because it doesn't make sense. In the second case, I was hoping to browse to another part of the site which does not use Aurelia.

plwalters commented 6 years ago

I'm on my phone so can't see the gist but that use case sounds exactly like what I've been asked for as well.

Alexander-Taran commented 6 years ago

workaround with ignoreUnknowRoutes no reply close/reopen it needed

EisenbergEffect commented 6 years ago

@davismj Can we close this?

plwalters commented 6 years ago

I left the comment in another issue before closing it but this makes pushState arguably unusable in real app scenarios such as putting an aurelia pushstate app inside of a server rendered page.

pndewit commented 6 years ago

@Alexander-Taran @EisenbergEffect I am using a dirty workaround as explained in https://github.com/aurelia/router/issues/527#issuecomment-349258603 but I would really like a real solution to the issue. The PR was merged, but then reverted and I hope the work started there can continue. I have an Aurelia application running in a larger web app and I need this Aurelia app to only react on route changes within its own scope.

Alexander-Taran commented 6 years ago

well can you handle Unknown routes? with -> doing nothing?

pndewit commented 6 years ago

If only I could lol. No, Aurelia Router then decides to route my app, I suppose, back to the last known route. That's why the workaround is to return a never resolving promise.

benstevens48 commented 6 years ago

Hi guys, Please don't close this. The with ignoreUnknowRoutes option (not sure on the status of this) solves a different problem in my opinion. I would still like a custom 404 page for my app, but I need the ability to add links (a tags) to different parts of the site which don't use Aurelia, or for downloads, so there needs to be a way of telling the router not to intercept clicks on certain links (a tags). (Adding specific entries to the router routes list is very cumbersome). Firstly I think the presence of a download attribute should bypass the router. Secondly, the presence of the target attribute does this in many other frameworks e.g. Angular 1, so I would suggest this. If you prefer there could be a built-in directive to tell the router to ignore the link.

EisenbergEffect commented 6 years ago

Try setting the link target to anything other than _self or top. See if that prevents the router from processing the link.

benstevens48 commented 6 years ago

Hi Rob, Well both _top and _parent work (_self or empty string don't and anything else results in a new tab). However, using _top or _parent are not good solutions as if your site is running in an iframe then this results in the target page being loaded in the root window or parent of the iframe (if permissions allow).

pndewit commented 6 years ago

And also, my Aurelia app is running in a larger SPA. Users that have visited the Aurelia child app have loaded in the Aurelia router code, making the router respond to all route changes (also completely outside the Aurelia app). Changing all links to have a target attribute there is not an option.

Alexander-Taran commented 6 years ago

So edge case ..

EisenbergEffect commented 6 years ago

You can implement a custom LinkHandler to address your specific needs. Take a look in the history-browser library. You can register a LinkeHandler with the DI and the history implementation will use that instead of the DefaultLinkHandler.

EisenbergEffect commented 6 years ago

Note: I think that if you set the link target to something custom, it will also work. Have a look at our DefaultLinkHandler to see the exact behavior that's currently implemented for link checking.

benstevens48 commented 6 years ago

Ok, thanks, I hadn't worked out you could do that. (Setting the target to something custom actually causes Chrome to open a new tab).

EisenbergEffect commented 6 years ago

@benstevens48 So, what I'd suggest, is take a look at the DefaultLinkHandler. We'd love to have a PR from you to add/address this scenario. I think that's the place it would go. In the mean time, while you are waiting for the PR to be merged and released, you could just plug in your own implementation. Sound good?

benstevens48 commented 6 years ago

Ok, I have made a pull request here - https://github.com/aurelia/history-browser/pull/36. It ignores links with a download attribute and adds a router-ignore attribute you can add to links to ignore them. I expect the pull request might need modifying.