router icon indicating copy to clipboard operation
router copied to clipboard

fix(react-router): `ResolvedSuspense` to always use `React.Suspense` never the `SafeFragment`

Open mta-trackunit opened this issue 1 year ago • 3 comments

If you create any hook that relies on a tanstack router hook - it becomes hard to test since the rerender recreates the elements again, I traced it down to this part in matches.tsx const ResolvedSuspense = !router.state.matches.length ? React.Suspense : SafeFragment It makes state changes with only the React.Suspense it works, dont really know if its intended or not? if so please explain how we could get around it for testing.

mta-trackunit avatar Jun 24 '24 13:06 mta-trackunit

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 816418330635e0607fcab5fa9b4d66ac06ae8297. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

nx-cloud[bot] avatar Jun 25 '24 11:06 nx-cloud[bot]

🚨 Tested this out locally using the kitchen-sink-file-based example.

This change breaks that example where, when on/dashboard, you click "Invoices" and navigate to /dashboard/invoices, and then try clicking on "Users" to navigate to /dashboard/users, the navigation never resolves.

Edit: This breakage is weird, since now when I retested it, the above case works, but this new one breaks.

  • Load localhost:3000
  • Go to dashboard
  • Go to invoices
  • Go to users
  • Go to a user from the list
  • Go to invoices
  • Try going to an invoice and notice the navigation doesn't actually resolve.

SeanCassiere avatar Jun 25 '24 23:06 SeanCassiere

So navigating from http://localhost:5173/dashboard/invoices to (using menu) http://localhost:5173/dashboard/users It navigates to http://localhost:5173/dashboard/invoices?usersView=%7B%7D

So it seems like tanstack router is behind 1 "tick" ... any idea how to debug that?

In examples/react/kitchen-sink-file-based/src/routes/dashboard.users.tsx


  React.useEffect(() => {
    navigate({
      search: (old) => {
        return {
          ...old,
          usersView: {
            ...old?.usersView,
            filterBy: filterDraft || undefined,
          },
        }
      },
      replace: true,
    })
  }, [filterDraft])

That seems to navigate - so it feels like tanstack router is behind and redirects so updating search on first render here navigates back to previous route (invoices).

A solution is to add to navigate function

 to: "/dashboard/users",

but it clearly hides some other bug...

mta-trackunit avatar Jun 26 '24 16:06 mta-trackunit

I gave up on fixing the this.state.resolvedLocation - I just added the "to" in the useEffect that fires on first render of the page. Without the "to" it will navigate back because this.state.resolvedLocation is still the "old" value - that is clearly a bug - but this is the workaround.

mta-trackunit avatar Jul 03 '24 21:07 mta-trackunit

@SeanCassiere The bug you found is also on main branch right now... I even hit it in our production site... adding "to": will fix it ... but it seems unrelated to this PR then... do you want me to revert the "to" fix ? or do we want this to be fixed on main this way

mta-trackunit avatar Jul 03 '24 21:07 mta-trackunit

@mta-trackunit FYI, I'm currently holding this ATM till #1907 goes through.

SeanCassiere avatar Jul 07 '24 00:07 SeanCassiere

The issues with the kitchen sink example should be resolved with #1968

SeanCassiere avatar Jul 17 '24 07:07 SeanCassiere

commit: 8164183

@tanstack/history

pnpm add https://pkg.pr.new/@tanstack/history@1822
@tanstack/react-cross-context

pnpm add https://pkg.pr.new/@tanstack/react-cross-context@1822
@tanstack/react-router

pnpm add https://pkg.pr.new/@tanstack/react-router@1822
@tanstack/react-router-with-query

pnpm add https://pkg.pr.new/@tanstack/react-router-with-query@1822
@tanstack/router-cli

pnpm add https://pkg.pr.new/@tanstack/router-cli@1822
@tanstack/router-devtools

pnpm add https://pkg.pr.new/@tanstack/router-devtools@1822
@tanstack/router-generator

pnpm add https://pkg.pr.new/@tanstack/router-generator@1822
@tanstack/router-plugin

pnpm add https://pkg.pr.new/@tanstack/router-plugin@1822
@tanstack/router-vite-plugin

pnpm add https://pkg.pr.new/@tanstack/router-vite-plugin@1822
@tanstack/start

pnpm add https://pkg.pr.new/@tanstack/start@1822
@tanstack/start-vite-plugin

pnpm add https://pkg.pr.new/@tanstack/start-vite-plugin@1822

Open in Stackblitz

More templates

pkg-pr-new[bot] avatar Jul 17 '24 07:07 pkg-pr-new[bot]

@SeanCassiere how come you removed the history: createMemoryHistory({ initialEntries: ['/'] }),

mta-trackunit avatar Aug 06 '24 08:08 mta-trackunit

@SeanCassiere I believe this is ready for release now?

mta-trackunit avatar Aug 06 '24 09:08 mta-trackunit

@SeanCassiere how come you removed the history: createMemoryHistory({ initialEntries: ['/'] }),

Mostly so we emulate the other tests and use the browser history.

SeanCassiere avatar Aug 07 '24 10:08 SeanCassiere

As mentioned in this discord thread, I've tested this locally with the basic, basic-file-based, kitchen-sink-file-based, and start-basic examples, and I have observed no visible regressions.

@schiller-manuel @tannerlinsley anything blocking this from your side?

you are linking to a private channel - so I cannot follow along :) - any update?

mta-trackunit avatar Aug 09 '24 07:08 mta-trackunit