Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(react-router): improve error handling for module loading failures in lazyRouteComponent #3262

Merged
merged 2 commits into from
Jan 28, 2025

Conversation

Sheraff
Copy link
Contributor

@Sheraff Sheraff commented Jan 28, 2025

Issues currently on main for lazyRouteComponent:

  1. The placeholder ReactNode used "while we wait for the page to reload" is not a valid react node

    return {
      default: () => null,
    }

    Based on the comment messages around, and the git history, I'm assuming this is a typo brought from when error handling was done at the module loading level instead of in-render (see e5e80af). There are 2 options now:

    • return null
    • throw new Promise(() => {})

    My vote would go towards the suspense one, because this is the least likely to flash content before the page reloads, and instead will continue the loading state already triggered by the navigation that brought us here.

  2. If the component re-renders "while we wait for the page to reload", the sessionStorage condition won't be met a 2nd time and we throw the error.

    // inside `Lazy` component
    if (!sessionStorage.getItem(storageKey)) {
      sessionStorage.setItem(storageKey, '1')
      window.location.reload()

    If Lazy re-renders, this condition won't be met the 2nd time around because we wrote to sessionStorage. This means that upon re-render, we're throwing the initial error that we were currently handling.
    I'm not 100% sure a re-render is possible here, so maybe there's not need to worry

  3. The technique used to differentiate a "module loading error" vs any other kind of error is only compatible with chromium browsers. Each browser throws a different error, because the specs say "rejects with an implementation-defined error":

    chrome: "Failed to fetch dynamically imported module: http://localhost:5173/src/routes/posts.index.tsx?tsr-split"
    firefox: "error loading dynamically imported module: http://localhost:5173/src/routes/posts.index.tsx?tsr-split"
    safari: "Importing a module script failed."
    

    So the isModuleNotFoundError implementation needs to be updated to handle those 3 types of errors.

  4. The key used for sessionStorage is not always unique to a specific file. As per the previous point, Safari does not include the imported module name anywhere (that I could see) in the Error object. So in the case of Safari, a user session can span across 1 deployment, but not 2 as the 2nd time we encounter a module loading issue we won't reload the page.

    I do not have a good solution for this:

    • It feels like the lazyRouteComponent function would have to take in the module name as a 4th param, but then we have to play with bundler stuff to obtain the module name and that's complicated.
    • Alternatively, maybe there's something to be done with a useId inside the Lazy component, since this is supposed to be stable across reloads IIRC After inspection, useId is not stable in this context, so it's not a possible solution to this problem

@Sheraff Sheraff changed the title fix(lazyRouteComponent): improve error handling for module loading failures fix(react-router): improve error handling for module loading failures in lazyRouteComponent Jan 28, 2025
Copy link

nx-cloud bot commented Jan 28, 2025

View your CI Pipeline Execution ↗ for commit b6a0690.

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ✅ Succeeded 5m 46s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 1m 19s View ↗

☁️ Nx Cloud last updated this comment at 2025-01-28 17:37:56 UTC

Copy link

pkg-pr-new bot commented Jan 28, 2025

Open in Stackblitz

More templates

@tanstack/arktype-adapter

npm i https://pkg.pr.new/@tanstack/arktype-adapter@3262

@tanstack/create-router

npm i https://pkg.pr.new/@tanstack/create-router@3262

@tanstack/create-start

npm i https://pkg.pr.new/@tanstack/create-start@3262

@tanstack/directive-functions-plugin

npm i https://pkg.pr.new/@tanstack/directive-functions-plugin@3262

@tanstack/eslint-plugin-router

npm i https://pkg.pr.new/@tanstack/eslint-plugin-router@3262

@tanstack/history

npm i https://pkg.pr.new/@tanstack/history@3262

@tanstack/react-cross-context

npm i https://pkg.pr.new/@tanstack/react-cross-context@3262

@tanstack/react-router

npm i https://pkg.pr.new/@tanstack/react-router@3262

@tanstack/react-router-with-query

npm i https://pkg.pr.new/@tanstack/react-router-with-query@3262

@tanstack/router-cli

npm i https://pkg.pr.new/@tanstack/router-cli@3262

@tanstack/router-core

npm i https://pkg.pr.new/@tanstack/router-core@3262

@tanstack/router-devtools

npm i https://pkg.pr.new/@tanstack/router-devtools@3262

@tanstack/router-generator

npm i https://pkg.pr.new/@tanstack/router-generator@3262

@tanstack/router-plugin

npm i https://pkg.pr.new/@tanstack/router-plugin@3262

@tanstack/router-vite-plugin

npm i https://pkg.pr.new/@tanstack/router-vite-plugin@3262

@tanstack/server-functions-plugin

npm i https://pkg.pr.new/@tanstack/server-functions-plugin@3262

@tanstack/start

npm i https://pkg.pr.new/@tanstack/start@3262

@tanstack/start-api-routes

npm i https://pkg.pr.new/@tanstack/start-api-routes@3262

@tanstack/start-client

npm i https://pkg.pr.new/@tanstack/start-client@3262

@tanstack/start-config

npm i https://pkg.pr.new/@tanstack/start-config@3262

@tanstack/start-plugin

npm i https://pkg.pr.new/@tanstack/start-plugin@3262

@tanstack/start-router-manifest

npm i https://pkg.pr.new/@tanstack/start-router-manifest@3262

@tanstack/start-server

npm i https://pkg.pr.new/@tanstack/start-server@3262

@tanstack/start-server-functions-client

npm i https://pkg.pr.new/@tanstack/start-server-functions-client@3262

@tanstack/start-server-functions-fetcher

npm i https://pkg.pr.new/@tanstack/start-server-functions-fetcher@3262

@tanstack/start-server-functions-handler

npm i https://pkg.pr.new/@tanstack/start-server-functions-handler@3262

@tanstack/start-server-functions-server

npm i https://pkg.pr.new/@tanstack/start-server-functions-server@3262

@tanstack/start-server-functions-ssr

npm i https://pkg.pr.new/@tanstack/start-server-functions-ssr@3262

@tanstack/valibot-adapter

npm i https://pkg.pr.new/@tanstack/valibot-adapter@3262

@tanstack/virtual-file-routes

npm i https://pkg.pr.new/@tanstack/virtual-file-routes@3262

@tanstack/zod-adapter

npm i https://pkg.pr.new/@tanstack/zod-adapter@3262

commit: b6a0690

@schiller-manuel schiller-manuel merged commit feb9a84 into TanStack:main Jan 28, 2025
5 checks passed
tannerlinsley added a commit that referenced this pull request Jan 29, 2025
* fix: scroll-restoration (especially on hydration mismatch)

* fix: more fixes

* fix canGoBack

* release: v1.97.25

* chore(root): upgrade `vitest` to `3.0.4` (#3257)

* chore(root): upgrade `nx` to `20.4.0` (#3259)

* fix(react-router): improve error handling for module loading failures in lazyRouteComponent (#3262)

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

* fix(start-server): remove debugging timeout (#3258)

* fix(start-plugin): invoke server implementation of createIsomorphicFn during SSR (#3268)

* feat(react-router): add `remountDeps` (#3269)

* fix: RELEASE_ALL

RELEASE_ALL

* examples: remove dep (#3270)

* release: v1.98.0

* checkpoint

* fix tests

* tests: fix

* fix: back to old onRendered

* fix: scroll-restoration (especially on hydration mismatch)

* fix: more fixes

* fix canGoBack

* checkpoint

* fix tests

* tests: fix

* fix: back to old onRendered

* Move utils to core

* fix: backwards compat ScrollRestoration

* fix: better versioning

* fix: reenable hash scrolling and window reset even without scroll restoration

* test(e2e): renable tests for router

* fix: backwards compat and docs

---------

Co-authored-by: Tanner Linsley <[email protected]>
Co-authored-by: Tanner Linsley <[email protected]>
Co-authored-by: Sean Cassiere <[email protected]>
Co-authored-by: Flo <[email protected]>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: Daniel Grant <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants