fix(router-core): update ErrorComponentProps to not support generic error type#4727
fix(router-core): update ErrorComponentProps to not support generic error type#4727leesb971204 wants to merge 3 commits intoTanStack:mainfrom
Conversation
Signed-off-by: leesb971204 <leesb971204@gmail.com>
|
View your CI Pipeline Execution ↗ for commit 524a5bc
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-plugin
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-plugin
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-server
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
packages/react-router/src/route.tsx
Outdated
| export interface UpdatableRouteOptionsExtensions { | ||
| component?: RouteComponent | ||
| errorComponent?: false | null | ErrorRouteComponent | ||
| errorComponent?: false | null | ErrorRouteComponent<any> |
There was a problem hiding this comment.
Is there a reason we wouldn't add the TError generic?
export interface UpdatableRouteOptionsExtensions<TError = Error> {
errorComponent?: false | null | ErrorRouteComponent<TError>There was a problem hiding this comment.
Is there a reason we wouldn't add the
TErrorgeneric?export interface UpdatableRouteOptionsExtensions<TError = Error> { errorComponent?: false | null | ErrorRouteComponent<TError>
If we make this change, wouldn’t it cause a type error in the errorComponent?
There was a problem hiding this comment.
Do you have any additional ideas regarding this?
There was a problem hiding this comment.
Not really sure to be honest. Its unclear to me why that would cause a compiler error, and I explicitly use code-based routing (createRoute) not file based. I suspect you would have to chase the generic error type onto UpdatableRouteOptionsExtensions as well, but now I'm starting to second guess following this through.
I would defer to @schiller-manuel.
There was a problem hiding this comment.
UpdatableRouteOptionsExtensions is not explicitly used in user code.
So if we declare it as UpdatableRouteOptionsExtensions<TError = Error>, the ErrorRouteComponent will always be inferred with Error as its generic type.
As a result, if the user specifies a type like ErrorComponentProps<string>, it will cause a type error—since Error is not compatible with string.
In conclusion, my personal opinion is that ErrorRouteComponent<any> is the appropriate way to declare it. What are your thoughts on this?
There was a problem hiding this comment.
UpdatableRouteOptionsExtensionsis not explicitly used in user code. So if we declare it asUpdatableRouteOptionsExtensions<TError = Error>, theErrorRouteComponentwill always be inferred withErroras its generic type. As a result, if the user specifies a type likeErrorComponentProps<string>, it will cause a type error—sinceErroris not compatible withstring.In conclusion, my personal opinion is that
ErrorRouteComponent<any>is the appropriate way to declare it. What are your thoughts on this?
Additional note:
The same type error also occurs with createRoute.
There was a problem hiding this comment.
I don’t think this approach can fundamentally solve the problem, as it changes the errorComponent’s props type to any.
|
In my view, changing the type of |
|
Even in TanStack Query, using If we follow the type CustomError = {
status: number
}
export const Route = createRoute<{
...
CustomError,
...
}>('/route')({
...
});Conclusion: The TanStack Query approach cannot be applied here, so the user must either perform error type narrowing themselves or declare the |
… parameter Signed-off-by: leesb971204 <leesb971204@gmail.com>
Signed-off-by: leesb971204 <leesb971204@gmail.com>
| export type ErrorComponentProps = { | ||
| error: Error |
There was a problem hiding this comment.
Since the scope of the errorComponent covers both errors from the loader and errors from the component itself, using the Error type is appropriate
There was a problem hiding this comment.
This seems to undo the work of making it handle throwing of non-errors. What if my loader calls some third-party API that internally does throw "Some string" or throw 50? Loaders can throw more than just Errors, and in some situations the user is not in control of what could be thrown.
There was a problem hiding this comment.
That’s right. But since in most cases the error is returned as Error, I think it makes sense to set the type to Error by default, and if needed, the user can manually override the error type using something like declare. Please refer to the link.
There was a problem hiding this comment.
is that the definite point of view of tanstack? i'm as well throwing something different that an Error and it is sad to not have the possibility to properly type the error for the ErrorComponent.
leesb971204
changed the title
leesb971204
changed the title
leesb971204
changed the title
3 participants
fixes #4686
more of #4691
I added a generic to
ErrorComponentPropsto allow specifying the error type,but since the generic wasn’t passed through in the
ErrorRouteComponenttype, I added explicit typing there as well.