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

RelayRouter public API #23

Open
Kingdutch opened this issue May 17, 2022 · 3 comments
Open

RelayRouter public API #23

Kingdutch opened this issue May 17, 2022 · 3 comments
Labels
enhancement New feature or request router Issue related to the router itself.

Comments

@Kingdutch
Copy link
Collaborator

This ticket writes down some thoughts what could be part of the public API. This is based on trying to use the router in an application.

Utils

RelayRouter__Utils implement hooks and helper functions that should probably all be available on RelayRouter itself. I think it may make sense to even skip the .Utils namespace here since they'll be commonly accessed.

I do notice that isRouteActive and useIsRouteActive are also on the individual route modules. Are they both supposed to be used or should only the route specific ones be accessed?

Bindings/History

RelayRouter__Utils.useLocation returns a RelayRouter__Bindings.History.location entry. This means that the bindings for History are needed to actually get anything from this value.

New functions

One thing I was trying to do was to create a login route that redirected to the current page. However, there's no easy way to assemble a path (including query string and anchor) for the current page from a location object.

@zth
Copy link
Owner

zth commented May 17, 2022

I do notice that isRouteActive and useIsRouteActive are also on the individual route modules. Are they both supposed to be used or should only the route specific ones be accessed?

Good question! I'd have to look that up to answer properly.

RelayRouter__Utils.useLocation returns a RelayRouter__Bindings.History.location entry. This means that the bindings for History are needed to actually get anything from this value.

Yes, this needs to be streamlined somehow. I'm actually thinking we should have and expose our own type, that we then map that to. So we don't have to expose the entire history module (which is an implementation detail).

One thing I was trying to do was to create a login route that redirected to the current page. However, there's no easy way to assemble a path (including query string and anchor) for the current page from a location object.

Hmm, could you elaborate? That sounds like a good thing to support.

@Kingdutch
Copy link
Collaborator Author

Yes, this needs to be streamlined somehow. I'm actually thinking we should have and expose our own type, that we then map that to. So we don't have to expose the entire history module (which is an implementation detail).

This was something I was thinking about, though maybe there's some merit to just exposing the history module itself? I can imagine apps maybe wanting to inspect what path the user took (perhaps for analytics purposes).

Hmm, could you elaborate? That sounds like a good thing to support.

Sure! So basically what I was trying to do (written here in JS rather than Rescript for simplicity) is:

function Navbar() {
  let location = useLocation()
  let isLoggedIn = LoginProvider.useIsLoggedIn();
  return (<nav>
    <a href="Route.makeLink()">Some page</a>
   {isLoggedIn
     ? <a href="/logout">Logout</a>
     : <a href=`/login?redirect=${makePathFromLocation(location)}`>Login</a>
  </nav>
}

The makePathFromLocation is missing in the router :) That same function could also be useful if you want to log the accessed full path for analytics purposes. I'd expect it to return the path, any query parameters and the hash in a formatted string :D

@Kingdutch Kingdutch added enhancement New feature or request router Issue related to the router itself. labels May 18, 2022
@zth
Copy link
Owner

zth commented May 19, 2022

Right, I think that's worth supporting since we do in fact expose the location object. Could you make a separate issue for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request router Issue related to the router itself.
Projects
None yet
Development

No branches or pull requests

2 participants