Refactor API

[Shmew]

Introduces a few changes for maintainability and easier use of api:

• Convert class component to function component
• Isolate props for router from router functions

  React.router [
  router.onChange <| fun _ -> ()
  router.children [ 
      Html.none 
  ]
  ]

• Add explicit elmish Cmds, eliminates the need for Router.execute

  Cmd.Router.navigate("some/path") : Cmd<'Msg>
  Router.navigate("some/path") : unit

• Unboxes ints as strings to reduce function calls

[Zaid-Ajaj]

Hi Cody,

I have been thinking about this and I some mixed feelings about it. Here is why:

• Convert class component to function component (if it ain't broken, don't fix it. The component implementation is already so small it doesn't really matter whether to use function components vs classes)
• Isolate props for router from router functions (Yeah but a breaking change doesn't outweigh the cosmetic improvement)
• Add explicit elmish Cmds, eliminates the need for Router.execute (this one was hard but it comes to down to the question of which version of the navigate functions should be more verbose, the elmish one or the pure React one and I am not a big fan of the Cmd prefix + Router module)
• Add prefix active pattern like what we use in the Feliz docs (isn't this basically | "Some" :: "Begin" :: "Path" :: rest ->?)
• Unboxes ints as strings to reduce function calls (yes please!)

Let's shrink this down to two main issues: explicit elmish Cmds and Isolate props for router from router functions.

If we go the breaking change route (no pun intended :wink:) then I propose to use Cmd.navigate instead of Cmd.Router.navigate and have Router.navigate stay here but returns unit like your initial proposal. Along with this change, we can include the fix for Isolate props for router from router functions

What do you think?

[Shmew]

Convert class component to function component (if it ain't broken, don't fix it. The component implementation is already so small it doesn't really matter whether to use function components vs classes)

I was thinking of this more along the lines of maintenance: keeping things more up-to-date. I believe the class component will work with concurrent mode, but this way we do know for sure. One thing is I'm pretty sure that the class component will not properly clean up the event handlers, or if it does work like that it would potentially remove listeners that are outside of this package.

Breaking change

I think a breaking change is worth it in the long run. It makes things easier to use from outside an elmish standpoint, as well as make it more clear what isn't pure. Removing the extra module is easy enough to do. Personally I like the extra name-spacing to avoid any potential conflicts with other libraries, but not enough to die on a hill for it lol.

Add prefix active pattern like what we use in the Feliz docs (isn't this basically | "Some" :: "Begin" :: "Path" :: rest ->?)

... learn something new every day, I thought it was only usable once as head::tail 😅.

If does look like the behavior is a fair bit different in terms of the generated code. I'm not sure how expensive list tail calls are in javascript, performance probably isn't ever going to be an issue with this kind of function. The active pattern does allow you to compose paths or pass in a value rather than explicit list destructuring. In addition it also lets you explicitly have a case for when it doesn't match that exact path. Up to you on this one.

[Zaid-Ajaj]

One thing is I'm pretty sure that the class component will not properly clean up the event handlers

The cleanup code will be the same actually whether it is in executed when a hook is disposed or when the component unmounts, I am keeping as is until someone comes up with an issue :)

I think a breaking change is worth it in the long run. It makes things easier to use from outside an elmish standpoint, as well as make it more clear what isn't pure. Removing the extra module is easy enough to do. Personally I like the extra name-spacing to avoid any potential conflicts with other libraries, but not enough to die on a hill for it lol.

It is always a balance, here I am assuming that someone probably won't have collisions on Cmd.navigate because people tend to use a single library for routing.

learn something new every day, I thought it was only usable once as head::tail 😅 If does look like the behavior is a fair bit different in terms of the generated code. I'm not sure how expensive list tail calls are in javascript, performance probably isn't ever going to be an issue with this kind of function. The active pattern does allow you to compose paths or pass in a value rather than explicit list destructuring. In addition it also lets you explicitly have a case for when it doesn't match that exact path. Up to you on this one.

Yeah I would rather not add patterns that look like something which is already there in the language

[Shmew]

I was mostly referring to the fact that since it doesn't provide the function in the removeEventListener. I can confirm this does not work as intended. This has probably never come up because the router should be at the top level, but it doesn't necessarily have to. With a function component it's very easy to stabilize the event handlers, here's an example where I've done this before. Maybe I should go ahead with creating a custom hook (something like useEventListener) for this pattern for the community to use.

Another thing I wanted to discuss is #16.

We could use a context to expose the router for React, and pass a Router type for Elmish. The registration of the router would then have an additional required parameter to register the router in the model: Router option -> Msg, then the commands would require a Router option. This makes the behavior consistent and much safer, Elmish users then can't shoot themselves in the foot or run into the issue @Dzoukr had. I've used this pattern in a couple libraries such as Fable.SignalR and Feliz.UseWorker, I've found it's successful in preventing illegal states. Additionally we could do the same with the React-only side of things by exposing a React.useRouter hook which would call the context and stabilize the object/functions enabling the same benefits.

This is also a good reason to re-implement the router as a function component, as we can use some of the optimizations currently available via hooks to ensure good performance and unnecessary re-rendering.

[Zaid-Ajaj]

I was mostly referring to the fact that since it doesn't provide the function in the removeEventListener. I can confirm this does not work as intended. This has probably never come up because the router should be at the top level, but it doesn't necessarily have to. With a function component it's very easy to stabilize the event handlers, here's an example where I've done this before. Maybe I should go ahead with creating a custom hook (something like useEventListener) for this pattern for the community to use.

Alright, fair enough 😄 function component it is

We could use a context to expose the router for React, and pass a Router type for Elmish. The registration of the router would then have an additional required parameter to register the router in the model: Router option -> Msg, then the commands would require a Router option. This makes the behavior consistent and much safer, Elmish users then can't shoot themselves in the foot or run into the issue @Dzoukr had. I've used this pattern in a couple libraries such as Fable.SignalR and Feliz.UseWorker, I've found it's successful in preventing illegal states.

I understand what you mean, but I think it makes the library more complicated than it should be. Making things simple and convenient is why I built this thing. However, to makes things EVEN simpler, I would love to have that React.useRouter hook 😍 which takes path mode as input? 🤔

[Shmew]

I understand what you mean, but I think it makes the library more complicated than it should be. Making things simple and convenient is why I built this thing.

Yeah I totally get that.

If we don't go that route these are the options as I see it:

• Pull the router outside of the react tree completely and make it a mailbox processor.
• Check before every command to ensure initialization has taken place (not sure how this would be done exactly).
• Keep it potentially unsafe (I feel like this is anathema from a Elmish POV).

For the React side of things I think the context would work very well, as we would then simply not expose the Router type except from that context. It would still need a function component so we can create the context producer and they can configure it. The hook would simply be React.useRouter() and it would return the Router type that exposes those functions that are available now in this branch, but provides the safety users would expect.

Anyways those are my ideas, just tell me what you want and that's what I'll do!

[Zaid-Ajaj]

It's less about what I personally want and more about trying to strike a balance. It is always good to re-evaluate things I never thought about twice, for example using a Router type or using React context. That said, I still believe that keeping things in this level of simplicity is the way to go, even if it is potentially unsafe. Using a context or using a dedicated type is not straightforward to understand from beginners point of view (especially if they haven't used React context before). Ideally when explaining routing to beginners, I want to say: "Routing is about the URL segments which are a bunch of strings that can change when the URL in the address bar changes." Done! No context, no special types that have to be passed around.

Following this concept, I think adding React.useRouter will be a huge win because of how it simplifies things where the router component itself wouldn't be needed anymore. I am thinking it could have this API

let currentUrl : string list = React.useRouter() // hash-based routing

let currentUrl = React.useRouter(HashMode.Path) // (or something similar for) path-based routing 

About Cmd.Router.navigate vs. Cmd.navigate: in my mind, whenever a library makes something "verbose", it is a way to discourage its use for certain times and only when the users really know what they are doing. That is why in a way I prefer Cmd.navigate because then we are not favoring the use Router.navigate (React-only) over the Elmish version. I hope that kinda made sense hahah. Sorry about the prolonged discussions :smile:

[Shmew]

Taking the router completely out of the react tree is an interesting idea.

In fact, I believe it also enables us to make it totally safe with that change for Elmish, as they wouldn't even need to create anything. We would just add a Cmd.configRouter with a signature of (RouterOptions -> RouterOptions) -> Cmd<'Msg>, a method with optional parameters (although it's not super idiomatic for Cmds), or the list style like we have now.

It's a bit awkward with React like this though, as they would need to use an effect to configure it at the root of their application, and then be forced to pass it around. At which point it would probably be more intuitive to use a component with context.

So it would look something like this:

Elmish

• SupervisorAgent

  • Starts up within the library itself.
  • Cmds would call it.
  • Handles creating a RouterAgent if not present.
  • Restarts a router when options change and disposes the old one.

• RouterAgent

  • Does all the things currently available.
  • Implements System.IDisposable to clean up event handlers.

This means if someone wants to use specific settings for their router it's their responsibility to set it in their init function, or before they send any messages, but also gives them the freedom to change things later.

Would look something like this:

type Model = ...
type Msg = 
    | UrlChanged of string list
    | ...

let init =
    someModel, Cmd.configRouter UrlChanged (fun o -> { o with HasMode = HashMode.Path })
// or
    someModel, Cmd.configRouter(UrlChanged, HashMode.Path)
// or
    someModel, Cmd.configRouter [
        router.onUrlChanged UrlChanged
        router.pathMode
    ]

let update msg state =
    | ...
        state, Cmd.navigate ... 

React

• Router component

  • Implementation similar to the current PR, but uses the agents with context under the hood.

• Router hook

  • Gets the context and any optimizations (React.useMemo) needed to ensure everything is stabilized.
  • Returns a Router that is essentially the same as the current Router.

let myChild = React.functionComponent(fun () ->
    let router = React.useRouter()    

    Html.button [
        prop.text "Click me!"
        prop.onClick <| fun _ -> router.navigate(...)
    ])

let render = React.functionComponent(fun () ->
    let page,setPage = React.useState []

    React.router [
        router.onUrlChanged setPage
        router.hashMode

        router.children [
            myChild()
        ]
    ])

This is technically a little bit more verbose, but it's very idiomatic to React, and is safe.

The reward for this is that we can add some goodies such as localized url observation. For example when working on a page that certain actions do not navigate, but instead modify the url to represent a persistent state of the application (like the Fable repl automatically changing the url instead of having to click a button to make the link sharable).

For example, in the myChild component we could do this:

let somePathFunc (segments: string list) : string = ...

let myChild = React.functionComponent(fun () ->
    let router = React.useRouter()    
    let text,setText = React.useState ""

    router.useObservation(somePathFunc >> setText)

    Html.div [
        Html.text text
        Html.button [
            prop.text "Click me!"
            prop.onClick <| fun _ -> router.navigate(...)
        ]
    ])

Sorry about the prolonged discussions 😄

Nonsense! It's totally worth it to make it as good as possible while still being intuitive to use.

[Shmew]

After our discussion on Slack I think I've got everything setup. I also found some additional points where code could be optimized so I've made those changes. I also inlined the rest of the Router type as they're just minor wrappers around other functions we're already calling, so it will help reduce bundle size.

[Zaid-Ajaj]

I have been trying out this implementation on my machine using the demo inside the repo and the router seems to be broken 😞 neither changing the URL via anchor tags nor commands triggers onUrlChange unless I do a full refresh

[Shmew]

Looks like I put too much trust in the tests, I didn't look too much at the app page since it looked so simple, saw the url change and figured it was good. I'll figure out what's going on

[Shmew]

Oh lol, I forgot to update the paket dependencies after fixing a bug with window listeners in the useListener library. A paket update fixes it and then it all works fine.

[Zaid-Ajaj]

I guess it is time to write better tests with RTL and asserting stuff after changing URLs etc.

Will look into it soonish