jashkenas / backbone

Give your JS App some Backbone with Models, Views, Collections, and Events
http://backbonejs.org
MIT License
28.1k stars 5.39k forks source link

History notfound event #4281

Closed jgonggrijp closed 1 year ago

jgonggrijp commented 1 year ago

This is a new feature that I factored out of an application I built at Utrecht University. It makes it possible to display a generic "404 not found" view whenever the URL changes but does not match any route known to Backbone.history. In the application, I hacked the feature in by overriding Backbone.history.loadUrl, which is an undocumented method. With this feature added to Backbone itself, that hack would no longer be necessary.

It was already possible to inspect the return value of Backbone.history.start() and display a 404 view if that value was false. However, there was no way to detect a non-matching URL that occurred after a hashchange or pushstate event, for example due to a coding error or a creative user hacking faulty URLs into the page. The notfound event addresses that gap.

Another way to look at it, is that notfound brings parity with the Backbone.sync events: notfound is to route as error is to sync.

I have some specific questions to the reviewer:

jgonggrijp commented 1 year ago

@GammaGames or @RayRaz does either of you have a "big" opinion?

GammaGames commented 1 year ago

I think it's a nice event to add, no objection!

Rayraz commented 1 year ago

Considering this usecase could be covered with a catch-all route it strikes me as a nice-to-have feature rather than a must-have.

Having said that, it's only very little code so unless micro-managing code size is a very high priority I don't see any strong arguments against this addition.

jgonggrijp commented 1 year ago

Yeah, I agree the event is somewhat redundant with a catch-all route. I didn't think of that at the time... :thinking:

While the added code size is negligible, a "con" might be that this feature introduces a new responsibility, something that must keep working as the surrounding code is edited. Then again, it doesn't seem like a difficult thing to maintain.

If I compare the notfound event to a catch-all route, I do think that the event is a little bit cleaner, so that would be a "pro". A catch-all route for 404 purposes does not seem like something that one would want to navigate to, nor does it seem appropriate to guard with an execute filter. It also seems intrinsically unrelated to any other routes that might be registered in the same router. Then again, a catch-all route is probably about as easy to use as a notfound event.

I could merge this, on the ground that all opinions so far lean to the mildly positive. I could also not merge this, on the ground that mildly positive opinions are not enough. There is something to say for requiring that new features enable workflows that weren't possible before.

Ah, I just realized there is actually such a workflow that is easier to implement with the notfound event. It is also the workflow I implemented in the application I factored this out of. Sometimes, a URL matches a known route but it should still lead to a 404 page. For example, consider /book/10/page/300, which matches the route pattern /book/:bookId(/page/:pageNum). If book 10 has only 160 pages, I might still want that route to lead to a 404 view. Doing so is easy by manually calling Backbone.history.trigger('notfound'), if I happen to listen for that event anyway. If I am handling unknown routes with a catch-all route instead, I will need to take additional steps to ensure that both situations are handled in the same way. Ironically, one possible approach in that case would be to trigger something like notfound on Backbone.history or some other global event bus in both situations.

I am now leaning towards merging, but I'll leave this open for a day, in case anyone can counter the above argument.

paulfalgout commented 1 year ago

Yep so I do something similar triggering a notfound like event. But I do this on a backbone.radio channel because I use multiple routers.. So if a router matches a route it can still be a 404, but the initial router handles unknown routes and listens for that event to display the 404. One particular bonus I think I utilize is that depending on the user's permissions some of my routers may or may not be running, so the routers are completely unaware of each other. However they would all have access to Backbone.history so I suppose that would be fairly the same as a radio channel.

In any case, I think my use case validates the pattern, even if I won't utilize the API.

jgonggrijp commented 1 year ago

In any case, I think my use case validates the pattern, even if I won't utilize the API.

Very reassuring, thanks!

Rayraz commented 1 year ago

This type of interesting insight into other people's usecases is why I like lurking here :smile: