fix: handle SSR errors in beforeLoad/loader #4301
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tanstack.com has been updated to use /src instead of /app, which means these broken asset links has to be updated too
… when called within `loader` (TanStack#4164)
… for TanStackRouterDevtoolsCore (TanStack#4126) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
The markdown list wasn't shown properly and I noticed the signup link was broken Before  After 
porting from alpha to main --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…ceEqualDeep` (TanStack#4237) Opening this PR for discussion, I'm very open to scope changes. This PR tackles two problems with `defaultStructuralSharing: true`: 1. symbol properties are not taken into account for "object equality" and are not copied over when creating a new copy 2. non-enumerable properties are not taken into account for "object equality" and are not copied over when creating a new copy Point 1. actually is causing some of our users a headache, see apollographql/apollo-client#12619 and https://community.apollographql.com/t/preloadquery-with-tanstack-router-loader/9100 . Users expect that they're able to pass an object with symbol properties over from a loader into components, but when having `defaultStructuralSharing` enabled, from the second object that is passed over, it creates a copy and strips off symbol properties. That new object without the expected symbol properties will then crash our `useReadQuery` hook. In other words, the first loader call returns ```js { refObject: { toPromise: Function1, [secretSymbol]: ImplementationDetail1 } } ``` the component receives ```js { refObject: { toPromise: Function1, [secretSymbol]: ImplementationDetail1 } } ``` and the second loader call returns ```js { refObject: { toPromise: Function2, [secretSymbol]: ImplementationDetail2 } } ``` but the component receives ```js { refObject: { toPromise: Function2 } } ``` `toPromise` is a different function, so it goes into the "clone" path, but skips the `secretSymbol` property. If we wouldn't have that `toPromise` property on there (and we're considering removing it!), it would even be worse to debug - users would always stay on the first `refObject` and never get the second, since both objects would be considered equal. On our side, passing these objects from loaders to components is actually an intended pattern, so this is causing quite a bit of problems - see this example from our docs (see [Initiating queries outside React](https://www.apollographql.com/docs/react/data/suspense#initiating-queries-outside-react)): ```ts import { useLoaderData } from 'react-router-dom'; export function loader() { return preloadQuery(GET_DOGS_QUERY); } export function RouteComponent() { const queryRef = useLoaderData(); const { data } = useReadQuery(queryRef); return ( // ... ); } ``` Problem 2. is purely hypothetical and we haven't encountered it - but it should probably be handled in some way. Both of these problems can be handled in two ways: * take these "special properties" into account for comparisons and copy them over (my naive approach here would lose the "non-enumerability" of these properties, more code would be necessary to keep that) * in `isPlainObject` just return `false` if an object has non-enumerable or symbol properties. This would opt out of structural sharing for these objects (which is probably perfectly fine) I'd be open for either of those solutions, and also for completely dropping the "non-enumerable" case for simplicity, but I would be very happy if we could get *something* in to help with symbol properties. In React Query, these values can be assumed to be serializable JSON, so the current implementation is perfectly fine, but with client-side loaders, users can just pass anything, and here it's causing quite the problem. Telling our users to situationally turn off structural sharing would be an educational nightmare.
…Stack#4257) Co-authored-by: Lleyton Morris <lmorris@nexigen.digital>
Custom links created by `createLink` would incorrectly infer `ref` values for non-intrinsic components (such as those with their own `ref` prop like the [documented example](https://tanstack.com/router/latest/docs/framework/react/guide/custom-link#basic-example)), which would result in the ref being "double-wrapped". That is: Before: ```typescript { // Effective LinkComponentReactProps<TComp> // ... ref?: React.Ref<React.Ref<HTMLAnchorElement> | undefined> | undefined // notice the ref has been "ref'ed" twice } ``` After fix: ```typescript { // Effective LinkComponentReactProps<TComp> // ... ref?: React.Ref<HTMLAnchorElement> | undefined> } ``` This patch, the equivilant applied directly to `link.d.ts` via `yarn patch`, has been our workaround and it appears to work well: <details> <summary>@tanstack-react-router-npm-1.120.3-f6c72d7c75.patch</summary> ```patch diff --git a/dist/esm/link.d.ts b/dist/esm/link.d.ts index f7a4dfc2678f3669ee48b97c22e4c69d2f3ff195..be0adda3bf4302466c3c4b4d73ff6c78474a237f 100644 --- a/dist/esm/link.d.ts +++ b/dist/esm/link.d.ts @@ -4,9 +4,9 @@ import { ValidateLinkOptions, ValidateLinkOptionsArray } from './typePrimitives. import * as React from 'react'; export declare function useLinkProps<TRouter extends AnyRouter = RegisteredRouter, const TFrom extends string = string, const TTo extends string | undefined = undefined, const TMaskFrom extends string = TFrom, const TMaskTo extends string = ''>(options: UseLinkPropsOptions<TRouter, TFrom, TTo, TMaskFrom, TMaskTo>, forwardedRef?: React.ForwardedRef<Element>): React.ComponentPropsWithRef<'a'>; type UseLinkReactProps<TComp> = TComp extends keyof React.JSX.IntrinsicElements ? React.JSX.IntrinsicElements[TComp] : React.PropsWithoutRef<TComp extends React.ComponentType<infer TProps> ? TProps : never> & React.RefAttributes<TComp extends React.FC<{ - ref: infer TRef; + ref: React.Ref<infer TRef>; }> | React.Component<{ - ref: infer TRef; + ref: React.Ref<infer TRef>; }> ? TRef : never>; export type UseLinkPropsOptions<TRouter extends AnyRouter = RegisteredRouter, TFrom extends RoutePaths<TRouter['routeTree']> | string = string, TTo extends string | undefined = '.', TMaskFrom extends RoutePaths<TRouter['routeTree']> | string = TFrom, TMaskTo extends string = '.'> = ActiveLinkOptions<'a', TRouter, TFrom, TTo, TMaskFrom, TMaskTo> & UseLinkReactProps<'a'>; export type ActiveLinkOptions<TComp = 'a', TRouter extends AnyRouter = RegisteredRouter, TFrom extends string = string, TTo extends string | undefined = '.', TMaskFrom extends string = TFrom, TMaskTo extends string = '.'> = LinkOptions<TRouter, TFrom, TTo, TMaskFrom, TMaskTo> & ActiveLinkOptionProps<TComp>; ``` </details> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Fix code-based splitting example. Change file name from `// src/posts.tsx` to `// src/posts.lazy.tsx`
Co-authored-by: Joshua Coffee <joshcoffee@gannett.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: SeanCassiere <33615041+SeanCassiere@users.noreply.github.com>
- Add generic error handling to existing redirect/notFound logic; rename handleRedirectAndNotFound to handleRouteError for clarity - Add 'invariantSource' to distinguish missing notFoundComponent invariant error from generic errors
Co-authored-by: Vianney Carel <vianney.carel@logfuse.fr>
|
thanks for tackling this. we need tests for this. can you please add a unit test? |
|
Hi @schiller-manuel , I only found some e2e examples for testing tanstack-start, so I added some e2e tests for #3462 |
meouwu-dev
changed the title
|
can you target the alpha branch please? I will review this afterwards then |
|
this is a bit hard to review, what are the actual changes? maybe you "just" need to cherry pick your changes on top of alpha and force push? |
|
Checked out the alpha branch and found out that #3462 has been fixed. I will close this PR. Thanks! |
|
are you sure? which pr/commit would have fixed that? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #3462