WIP: added in a new fallback route feature.

[jwoertink]

Fixes #728

I labeled this as WIP since I'm sure there will be a few comments on this. Plus I'm going to implement this in to an app to feel it out a bit and see what might be missing.

This adds in the feature of a fallback route using

class SomeAction < BrowserAction
  fallback do
    text "whatever here"
  end
end

This also removes the Lucky::Router::INSTANCE constant to a new private class property due to an issue with the specs where each spec would carry over the instance variables. Doing it this way allows us to reset that internally. It should be noted that this reset! is not intended to be a public API, and anyone else reading the code that uses it should expect nothing less than an explosion 😆

[paulcsmith]

Very cool. So stoked for this. Can't wait to hear how it goes in your app 🤞

[paulcsmith]

This looks great. Once you’ve tested in your app and it works, could you revert the original changes and ping me? Then I’ll merge this in 🎉

[jwoertink]

wtf? apparently I don't know how to git this morning. 😕

[jwoertink]

@paulcsmith ok those router changes are removed from this PR now. Do you want me to send another PR to re-implement those with the reset? Or just open an issue to handle later?

[paulcsmith]

Looks perfect 👍

I think we can leave the Router as-is now that we don't need to reset it anymore. The INSTANCE constant really is a constant again so is probably ok. What do you think?

[paulcsmith]

Oh and did you try this out on your app and can confirm it works?

[jwoertink]

I like leaving it as a constant; however, I think maybe the specs need to be a bit more robust. The fact that each spec instance carries over information from a previous spec isn't good. But I think that can be a low priority situation since it doesn't really affect an app (that I can think of)

[paulcsmith]

luckyframework/website#224

[paulcsmith]

@jwoertink Yeah that makes sense. I think an issue to handle it later would be good. Making them more robust would be great.

Also what do you think about catch-all v. fallback? Example catch_all do/end and CatchAllAction? I don't have a strong opinion, but I figured I'd ask in case one sounds better than the other

[jwoertink]

It does work on my app locally. This app isn't in production just yet, but it's working as intended. There's a weird issue now with the fact that lucky no longer throws a 404 on GET routes when the fallback is added. Though, I spoke with a few different react devs and they say that's sort of common. I guess with rails it's pretty common to use a glob route, and have the SPA handle the routing...

And on that note, the SPA will handle non-existent routes (provided you handle that yourself), it's just lucky returns a 200 as opposed to 404. I think that's ok though, just maybe needs to be documented?

[jwoertink]

I do like catch_all, though I wonder if that would imply for ALL HTTP methods? as opposed to just GET? We can always try one and then rename later if people think it's confusing?

[paulcsmith]

Yeah let’s just stick with fallback for now :)

And that is one of the bummers about the SPA life. Though I’d suggest something like this in the FallbackAction most of the time:

fallback do
  if json?
    raise Lucky::RouteNotFoundError.new
  else
    # do your thing
  end
end

That way JSON responses still get a 404. That can be documented in the guides

[paulcsmith]

I think this looks good. I’ll go ahead and merge but if you try it and find any issues, I’d be happy to open review a new PR