Skip to content
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

Frontend Routing #197

Merged
merged 24 commits into from
May 16, 2023
Merged

Frontend Routing #197

merged 24 commits into from
May 16, 2023

Conversation

davesmith00000
Copy link
Member

@davesmith00000 davesmith00000 commented May 2, 2023

The aim of this PR is to build frontend routing into Tyrian.

The plan is to do the same as Indigo, i.e. depending on which initial trait you extend you get different behaviour. In this case, SinglePage is the replacement for TyrianApp, and MultiPage will give you almost the same API, but plumbs the routing into your app for you.

@davesmith00000 davesmith00000 self-assigned this May 2, 2023
@davesmith00000
Copy link
Member Author

Build is failing on unrelated test cases...

@davesmith00000
Copy link
Member Author

Progress so far: I've got the two entry points defined, and I've simply lifted the existing Navigation module (which I have not attempted to improve) to provide location hash based routing just to prove it works.

Ugly, basic, but working.

The next thing will be to replace the hash based navigation with something more general purpose, and to clean up the API so that the requirement is for you to provide a simple function like this:

def router: Location => Msg 

@davesmith00000 davesmith00000 requested review from hobnob and bilki May 2, 2023 07:19
Copy link
Collaborator

@bilki bilki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just added a few comments 😄

tyrian/js/src/main/scala/tyrian/TyrianRoutedAppF.scala Outdated Show resolved Hide resolved
sandbox/src/main/scala/example/Sandbox.scala Outdated Show resolved Hide resolved
tyrian/js/src/main/scala/tyrian/TyrianRoutedAppF.scala Outdated Show resolved Hide resolved
tyrian/js/src/main/scala/tyrian/TyrianRoutedAppF.scala Outdated Show resolved Hide resolved
indigo-sandbox/src/main/scala/example/IndigoSandbox.scala Outdated Show resolved Hide resolved
@davesmith00000 davesmith00000 marked this pull request as ready for review May 5, 2023 07:44
@davesmith00000
Copy link
Member Author

You may see failing builds here, but it's a flaky test... #199

bilki
bilki previously approved these changes May 5, 2023
Copy link
Collaborator

@bilki bilki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, nice as a starting point.

tyrian/js/src/main/scala/tyrian/TyrianRoutedAppF.scala Outdated Show resolved Hide resolved
@davesmith00000
Copy link
Member Author

@hobnob & @bilki: I've resolved the old discussion points, either by fixing them or because they're out of date - hopefully I didn't missing anything.

(cc @DeamonDev in case you have any thoughts on this. Note that I'm not planning to bake any third party url parsing libs into Tyrian directly, but it would be ideal if this solution optionally worked with libs like url-dsl, as you're doing in #195.)

This all works, but it needs more thought.

Problem 1 - Hyperlinks

The first (and maybe main) problem is that you need to set hyperlinks like this at the moment:

a(href := "#", onClick(Msg.NavigateTo(page)))("my link")

Rather than the more desirable (I think? Probably better for accessibility too?):

a(href := "/my-page")("my link")

To make the latter work, I think we'd have to somehow set preventDefault on all a tags, effectively:

<a href="/my-page" onclick="event.preventDefault()">my link</a>

The catch with that is that I think I'd have to patch it into the runtime somewhere as a special case. 🤔

Problem 2 - Routing UX

The second problem is the routing function. I thought this was all going smoothly until I realised just how nuanced routing can potentially get. 😅

At the moment I've dodged the problem by introducing a Location type and basically making all routing decisions the users problem. Want to work on hash changes? Check the hash field. Want to use full paths? Carry on. ...but I feel like we could do better in terms of user experience somehow...

Do we need to do that now? Could it be a future enhancement based on user feedback? Looking up how other frameworks do this is a bit of a minefield. Recommended reading suggestions welcome.

Problem 3 ...what have I missed?

I'm super confident that there is more complexity here I'm missing because I'm sort of building this in abstract without a concrete use case. Suggestions welcome.

@hobnob
Copy link
Member

hobnob commented May 6, 2023

Looks good Dave!

You're right that the second option for a tags would be more accessible, and user friendly. Also means that you don't break how links work (for example the user can ctrl+click to open a link in a new tab).

The question of how to achieve it though is a little tricky 🤔 I wonder if we could modify the a so that it took a new attribute called route. You could then write: a(route := Route('/page/{1}/{2}', 'someParam', 'someParam2'))("My text") and Tyrian would render it with the correct markup (in this case <a href="/page/someParam/someParam2" onclick="event.preventDefault()">My text</a>)

Additionally, you could also create a new Route object that takes a number of additional parameters, such as base URL and route parameters. A simple implementation might look like this (I haven't compiled it, so it's probably wrong):

final case class Route(baseUrl: String, path: String, routeParams: Seq[String]) {
}

object Route:
  def apply(path: String, routeParams: Seq[String]): Route =  Route("", path, routeParams)
  def apply(path: String): Route = Route("", path, [])

The benefit here would be that you could do a check against a string (such as a URL) against a route, and could also use a base URL in case you were using Tyrian in a subfolder on a main site (such as a blog) without having to change loads of code if the Base URL changes.

Hope that helps 🙂

@davesmith00000
Copy link
Member Author

davesmith00000 commented May 7, 2023

Thanks @hobnob - I've mulled it over and it's a good idea as a way to give us a hook to decide what to do next, I will need something like that. I would like to avoid adding a non-standard entity into the markup however (if possible). Ideally if you can HTML, you should be able to Tyrian (and if you can't HTML, you can google HTML and apply almost 1:1). (Have we already added non-standard stuff with the events? Maybe? Gah! 🤔)

As usual, all roads lead to Elm. I've had a bit of a snoop around what they do (I haven't read the code, just mooched around the docs to borrow wisdom) and if applied directly, I think it goes something like this:

  1. Intercept a tags with an href.
  2. If the tag has an onClick, do nothing, our programmer has a plan (and preventDefault will happen by default).
  3. If there is no onClick then the user intends a normal link, so we add an onClick that:
    a. Applies preventDefault.
    b. Captures the link and turns it into a type similar to the new Location type, but that is an enum of Internal and External.
    c. Note: There is a problem here to overcome in so much as we don't have access to the user's Msg type, so we may need to wrap it up or something.
  4. The data is then passed to the router, which now runs before any action is taken on the link.
  5. The router is modified so that the signature is something like def router: LocationEnum => NavTo[Msg]
  6. This effectively captures all unhanded links in the App, gives them to the user, and lets them decide whether to just follow them or to modified their app based on the route.
  7. The NavTo type above is then use to decide what sort of pushstate / location update to do.
  8. Open question: What happens with the Sub in this scenario? I think it'll come out in the wash and we'll still need it (in case a link is entered manually for example) but it's relationship with the router is a little unclear. However it may be that we don't reorder when the router is run though (point (4)) and then it's all ok, we'll see how it pans out.

This isn't a perfect translation of the Elm solution, but I think this is the broad strokes of what the shape of the solution needs to be. No doubt I'll discover more problems along the way.

On reflection, I think some of this goes towards a suggestion @bilki had in an earlier comment around having some generic nav types.

@davesmith00000
Copy link
Member Author

This version has a few warts, but appears to work correctly. There's a couple of TODOs to address (called out in comments), and the only other thing is that the initial routing has to happen last, by which time we've briefly (like a flicker) rendered the homepage even if we want some other page.

@davesmith00000
Copy link
Member Author

Also, I suspected this wouldn't work so I've added an external link to the sandbox, and clicking that produces an error.

Uncaught DOMException: The operation is insecure. index.cb180e2c.js:19134
    callback sandbox-fastopt.js:21147
    apply__O__O sandbox-fastopt.js:39300
    apply__sci_Seq__Lsnabbdom_EventHandler sandbox-fastopt.js:17115
    apply__O__O__O sandbox-fastopt.js:39324
    handleEvent__Lorg_scalajs_dom_Event__V

@davesmith00000
Copy link
Member Author

davesmith00000 commented May 13, 2023

Getting there, two outstanding issues that I know about:

  1. Following anchor/hash links isn't working in the sandbox, this is because you just get the path from the anchor onwards. Just need to decide how to handle that (and whether it's a user problem or an engine problem.

  2. Following external links is producing an error: Uncaught DOMException: The operation is insecure.

UPDATE:
Decided that (1) is a user problem until I have a real use case to show me it isn't. (2) has been fixed.

@davesmith00000 davesmith00000 requested review from hobnob and bilki May 15, 2023 18:51
@davesmith00000
Copy link
Member Author

davesmith00000 commented May 15, 2023

I think I'm done with this now. The end result is that TyrianApp is back and everyone has to do some routing. However I've made it pretty easy (I think).

Your Tyrian web app will now need to implement def router: Location => Msg, and there are a few ways to do that (note that in all cases, you're told if the link is considered internal or external):

  1. You can use the helpers if your needs are simple, for example, this one ignores all internal links and just routes external ones. Note that you need to provide suitable msg's:
def router: Location => Msg = Routing.externalOnly(Msg.NoOp, Msg.FollowLink(_))

I've also made Routing.basic (simple string match) and Routing.none variations.

  1. Alternatively, you can do a full match, like this:
  def router: Location => Msg =
    case loc: Location.Internal =>
      loc.pathName match
        case "/"      => Msg.NavigateTo(Page.Page1)
        case "/page1" => Msg.NavigateTo(Page.Page1)
        case "/page2" => Msg.NavigateTo(Page.Page2)
        case "/page3" => Msg.NavigateTo(Page.Page3)
        case "/page4" => Msg.NavigateTo(Page.Page4)
        case "/page5" => Msg.NavigateTo(Page.Page5)
        case "/page6" => Msg.NavigateTo(Page.Page6)
        case _        => Msg.NoOp

    case loc: Location.External =>
      Msg.NavigateToUrl(loc.href)

The Location type comes with all sorts of information in it for you to decide how to route, it is similar to the JavaScript Location type.

I imagine that you could also decide to pull in some fancy url parsing library to build a clever router here if you liked.

Once you've done your routing, you'll want to use some of the new Nav cmds. For internal links, you can use Nav.pushUrl to update the address bar, and for external, you can use Nav.loadUrl to tell the browser to follow the link.

@davesmith00000 davesmith00000 merged commit cf8772d into main May 16, 2023
@davesmith00000 davesmith00000 deleted the frontend-routing branch May 16, 2023 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants