feat: phase 2 init#13084
Conversation
satya164
left a comment
There was a problem hiding this comment.
I'd also put a nested navigator (probably native tabs) in the example, so we test with more realistic situation.
| // TODO: useLayoutEffect? | ||
| React.useEffect(() => { | ||
| if ( | ||
| initialLoaderFiredRef.current || | ||
| staticContext == null || | ||
| !staticContext.isOutermost | ||
| ) { | ||
| return; | ||
| } | ||
| initialLoaderFiredRef.current = true; | ||
| const composedLoader = UNSTABLE_getLoaderForState( | ||
| staticContext.tree, | ||
| state | ||
| ); | ||
| if (!composedLoader) return; | ||
| staticContext.abortControllerRef.current?.abort(); | ||
| const controller = new AbortController(); | ||
| staticContext.abortControllerRef.current = controller; | ||
| composedLoader(controller.signal).catch(() => {}); | ||
| // Only the initial state | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [staticContext]); |
There was a problem hiding this comment.
What's this needed for? Initial mount?
For mount, the screen itself should load the data. We should only call loaders on navigation actions.
This means when using React Query, the useSuspenseQuery will trigger the data fetching.
| staticContext.abortControllerRef.current?.abort(); | ||
| const controller = new AbortController(); | ||
| staticContext.abortControllerRef.current = controller; | ||
| loader(controller.signal).catch(() => {}); | ||
| React.startTransition(() => { | ||
| setState(result); | ||
| }); |
There was a problem hiding this comment.
I'm unsure if we should always abort other data fetches if a new one begins. For Web, it makes sense as a single screen is rendered at one point. For native, not sure.
If I push one screen, and then the next before the previous one finishes, if we abort the previous screen's loader, then when you go back, it'll be in error state, which is not ideal behavior.
Maybe aborting makes sense if data isn't fetched yet and the screen gets unmounted. Or maybe we shouldn't bother with aborting at all.
In any case, if we keep abort behavior, we should clear the abort controller in finally for loader, so we don't unnecessarily call abort signal listeners.
| React.startTransition(() => { | ||
| setState(result); | ||
| }); |
There was a problem hiding this comment.
The startTransition may mess with native state updates. For now lets move it to callsite that does navigation.navigate. And create a ticket to review how to integrate well. I'm thinking we can integrate it in useLinkProps by default.
| React.useEffect(() => { | ||
| if (!isOutermost) return; | ||
| return () => { | ||
| abortControllerRef.current?.abort(); | ||
| abortControllerRef.current = null; | ||
| }; | ||
| }, [isOutermost]); |
There was a problem hiding this comment.
Same as the other abort related comment. Maybe we don't need it. I'd put it for investigation as another step.
| <ErrorBoundary | ||
| onReset={() => { | ||
| config.shouldFail = false; | ||
| queryClient.clear(); |
There was a problem hiding this comment.
Not sure it makes sense to reset the whole query cache when error occurs.
For ErrorBoundary, I'd also wrap the screen (in screenLayout) instead of the navigator, similar to suspense. When error happens, for retry, the user should just call refetchQuery instead of resetting.
| Detail: { | ||
| screen: DetailScreen, | ||
| linking: 'detail', | ||
| UNSTABLE_loader: async () => { | ||
| await fetchData('detail-data', 1000); | ||
| UNSTABLE_loader: ({ params, signal }) => { | ||
| const delay = getDelay(params); | ||
| signal.addEventListener('abort', () => { | ||
| queryClient.cancelQueries({ queryKey: ['detail-data', delay] }); | ||
| }); | ||
| return queryClient | ||
| .ensureQueryData(detailQuery(delay)) | ||
| .then(() => undefined); | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Use createNativeStackScreen for the screen and use a pattern for linking so params has the proper type, instead of a getDelay helper.
| UNSTABLE_loader runs before the screen mounts. The screen reads via | ||
| useSuspenseQuery, so the same fetch is shared between the loader and the | ||
| screen through TanStack Query's cache. |
There was a problem hiding this comment.
Codex loves putting description of example inside the example. Lets keep it more realistic.
| (params as { delay?: number } | undefined)?.delay ?? 1000; | ||
|
|
||
| function HomeScreen() { | ||
| const navigation = useNavigation<typeof LoaderStack>(); |
There was a problem hiding this comment.
Use the name LoaderHome for the home screen and then:
| const navigation = useNavigation<typeof LoaderStack>(); | |
| const navigation = useNavigation('LoaderHome'); |
|
|
||
| function DetailScreen() { | ||
| function LoadingFallback() { | ||
| debug.wasLoadingVisible = true; |
There was a problem hiding this comment.
This is a bit weird. We shouldn't be mutating things that are rendered during render, even if for the example. The closer we keep the example to real-world usage, the closer our testing is to identify gaps in DX as well as implementation issues.
| const loader = staticContext | ||
| ? UNSTABLE_getLoaderForState(staticContext.tree, result) | ||
| : undefined; |
There was a problem hiding this comment.
Also, I missed this - UNSTABLE_getLoaderForState only handles loading data for focused route. But if you add multiple routes at once, or preload a route etc., it won't work. I think we need a diff based approach, see all new routes that are added, not only focused routes.
Screen.Recording.2026-04-25.at.23.17.55.mov