frontarm / navi

🧭 Declarative, asynchronous routing for React.
https://frontarm.com/navi/
MIT License
2.07k stars 71 forks source link

strongly typed useCurrentRoute? #111

Open jsnelgro opened 5 years ago

jsnelgro commented 5 years ago

Hi, I'm wondering how to use useCurrentRoute().data without losing the type safety of typescript. For the time being, I'm not using the getData function in my routes, and instead I'm just using an async getView that also does my data fetching (then just passes the data as props).

Will I run into any roadblocks with this approach or lose any important functionality? Do you have any documentation on using typescript with Navi? Thanks!

pocesar commented 5 years ago

would require that the useCurrentRoute receive a generic T parameter that defaults to any as in:

export function useCurrentRoute<T = any>(...) => Route<T>

the interface would have to be changed to have generics as well (some aren't) a lot of places that it could have that without breaking backward compatibility, instead of relying in typecasts like

const route = useCurrentRoute().data as SomeData

would then be

const route = useCurrentRoute<SomeData>().data

the hook uses useContext underneath, and wouldn't be able to assume the right type of the .data parameter unless it also typecasts the useContext with your "T"

jamesknelson commented 5 years ago

Currently the only real advantage of using data instead of just passing props to a view is that the data can be accessed from anywhere -- even above the route's view. It's a bit like a badly named global context.

Re: typing, I've run into as well. I've basically given up on type safety of route.data for this version of Navi, as like @pocesar mentioned, it's implemented with context so the best you can do is tell useCurrentRoute() what type you want.

To get typing right, I think we'll need to modify the route.data interface a little so instead of using string keys, data can be assigned to an object like the one returned by React.createContext() -- and then can be pulled into the app with some useContext() like hook. It could even be designed to use Context itself. This is no means final, but something like this could work

let Course = React.createContext<{ name: string }>()

withData(Course, async () => {
  return { name: string }
})

function View() {
  let course = useRouteData(Course)
}

Obviously the names need to change, but this would solve the type problem and make the difference between data and view props a little more clear.

Would love to hear your thoughts.

pocesar commented 5 years ago

although the infer keyword would make it easier to make it safer, I think that shuffling around the code just for it to be type-safe out-of-the-box (like, instead of applying your typecasts), isn't very productive IMHO.

people needed generic components, TS team implemented them in TSX (that you can do <SomeComponent<Typed> prop={1} />), but createContext is a different beast, it expects the data to be more or less stable and isn't actually a good place to store 'current view' data. so, in reality, the typings for the current view data will never be good enough, because it changes as routes changes, and it can be anything, really, so if you rely entirely on type-safety, you'll have trouble with undefined behavior that totally depends on either your child views (and if they even throw something) or upstream problems, with malformed data

I guess I overdid myself here haha

jamesknelson commented 5 years ago

I guess I overdid myself here haha

It's great, I love hearing well thought out opinions 🙌 I see your point about current view data not really being constant across the app too, which is why I basically gave up on typing this stuff.

I think perhaps the difference here is how we see what route.data is for? For me, I mainly use it for global data is reasonably well typed across all routes, while I just pass props to a view element for anything that is view specific.

The other thing about the current situation with string keys for data is that it's possible to have collisions if you're not careful with naming.

I feel like it's certainly possible to improve on the current situation before 1.0 in any case. Any ideas would be welcome!

pocesar commented 5 years ago

couldn't a WeakMap be used? since the routes are initialized outside of react, it would be safe to use them as the key. that would also allow to fetch metadata from the route themselves if needed, like:

const MainRoute = route(/*...*/)

const router = mount(MainRoute)

const App: React.FC = () => {
   const data = useRouteData(MainRoute) 
  /* thus, this would be able to be strongly typed, much 
  closer to the withData helper without the need of a context, 
  AND would be free of hassle for non react integration */
}

but I assume the routing part would have to be swapped. the path would need to be inside MainRoute and not inside mount anymore, but that would encapsulate EVERYTHING you'd need for that specific route. the need for a context would then be gone!

going further, the type of the data could then be infered from the route call, the useRouteData would then be something like:

function useRouteData<T extends Route<any>>(route: T): T extends Route<infer U> ? U : unknown {
   /**/
}

making it really really type safe, at least, at the compile time. which you would also be able to extract from promises as well. 'minimal' PoC http://www.typescriptlang.org/play/#code/JYOwLgpgTgZghgYwgAgEoHsCukA8AVAPmQG8AoZC5AEzjDgH4AuZPcygcwjABFaHmAFAEpkAXiIAFKOgC2wAM4R8BUgF9SpGJhAIwwdCGSZFGbBF518yCAA9IIKvLRZccEAE8CBAdLPM8Qv7WdhAOTqa4oDDQyACqRPRxyMzaANYg6ADuhmSUAPQAVAV5bMhQXJhQhiCYADa1yHBObu5qGlo6egZlLkp4wfaOzmY4LV4+vf6BLCSlZRVVPWZtpAgG8mDIALJwoBEookuQAsTUfMwA5E1UMBfIqkIaayAbyAoAggDKYFCg7GJGEy9CxwAQ7Pa9R6rdabd4ZMAAC2g+wBvmOp04PHOyGEYkk0jkigAdOV5OhagA3CACAAMIgeTxhb3k7wAcpgZAAjGKHYwQfYggRw9CI5GQxkvTaxEDpLIgFGHNHU4gM6GS5nS2XZAF8gV8ASajLZfYiPJ5ZAwdBQJBOUWAlDoGCNZBgdwABwgCCam2MfyMMqNhm5lvK9tIQA

(hover over isA* variables on the left side)