[RESOLVED] Async Route vs Sync Route: Signatures are error-prone
I am new to Deno Fresh and I ran into a strange bug (on my side), when changing a sync route to an async route, because I was getting the Request instead of the RouteContext/PageProps.
As in the docs, I was using exported default functions (instead of typed Fat-Arrow-Functions) to define my routes. When changing a route to async, the compiler would not (and could not) warn me, that I am now accessing the wrong property, because at runtime the AsyncRoute will get an extra parameter "Request" as the first (!!!) parameter.
A few questions pop up there for me:
- Why don't we pass in the request in sync routes? I think it could be helpful in a sync route, too.
- Why do we have such an incompatible signature for two very similar things (sync vs. async route)? I would add it as second parameter, as most routes do not need it (even the example in the docs happily ignores it) or even better, just add it to the route context.
- Why don't we at least explicitly hint users in the docs in a very obvious way, that async routes and sync routes have an incompatible signature?
I know the first two would be a really nasty breaking change, because the compiler/IDE would not be of much help, but updating the docs could be helpful.
2 Replies
we should really nudge people more towards
defineRoute() etc. This lets the IDE infer the correct types so that issues like this can't happen
Why don't we pass in the request in sync routes? I think it could be helpful in a sync route, too.Sync routes are plain Preact components. Because of that they only receive one "props" argument. We could certainly add the request to
props.request or something
Why do we have such an incompatible signature for two very similar things (sync vs. async route)? I would add it as second parameter, as most routes do not need it (even the example in the docs happily ignores it) or even better, just add it to the route context.That's fair criticism. I'm not sure if we can change it though as changing it would be a breaking change. In more bigger projects the request argument is used pretty frequently though.
Why don't we at least explicitly hint users in the docs in a very obvious way, that async routes and sync routes have an incompatible signature?Check out the
defineRoute() helper function. Seeing that you missed it makes me wonder if more people miss it. We should point to the define helper more "aggressively" in our docs and make that the default there
Simplifying all this is definitely something we should plan to for Fresh 2.0
The more I think about the more I think you're right, we should just conceptually provide one "context" object where everything is one. That would allow us to consolidate all these different signaturesMarvin, thanks for the hint, I indeed overlooked the defineRoute helper (one of those TL;DR mistakes) and was about to write one myself (that would also put the request into the context). I will directly try the helper.
Tried it out, refined it for my use case, implementing a definePage helper that already fetches my user session from the cookies and adds it to the context and voilá, everything works like a breeze and will keep me from making stupid mistakes.
Thanks again for the quick response. I think it would be good to introduce the defineRoute helper already with the initial examples in the docs for those hands-on readers like me.