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

Deferring expense logs loader data not working when navigating expense detail pages. #129

Open
andrelandgraf opened this issue Dec 13, 2023 · 4 comments
Assignees
Labels
bug Something isn't working with solution Tag for bug tickets that document the solution

Comments

@andrelandgraf
Copy link
Collaborator

andrelandgraf commented Dec 13, 2023

Describe the bug

Deferring expense logs loader data does not work when navigating between expense detail pages.

To Reproduce

Steps to reproduce the behavior:

  1. Finish chapter 13.
  2. Delay (setTimeout) the expenseLogs database query for better visibility (see p. 247).
  3. Use the expense overview page (expenses list) to navigate between different expense details pages.
  4. Notice that the navigation to the expense details page only happens after the expenseLogs data is fetched, not deferring the expenseLogs fetch request.

Expected behavior

When navigating between expense details pages, the navigation should happen after the expense is fetched, but should not await the expenseLogs promise. Instead, it should render the expense details page and show the fallback loading indication for the expenseLogs section.

Actual behavior

The navigation only happens after the expenseLogs promise resolves, not deferring the promise.

Additional context

When navigating from any other page to an expense details page (e.g., the income routes) or when performing a full-page reload, the deferring of the loader data works as expected. The bug only occurs when navigating between detail pages.

@andrelandgraf andrelandgraf added the bug Something isn't working label Dec 13, 2023
@andrelandgraf andrelandgraf self-assigned this Dec 13, 2023
@andrelandgraf
Copy link
Collaborator Author

andrelandgraf commented Dec 13, 2023

Solution

We have to create a new Suspense boundary when navigating between expense details pages using the key property:

<Suspense fallback="Loading expense history..." key={expense.id}>
  <Await resolve={expenseLogs} errorElement="There was an error loading the expense history. Please try again.">
    {(resolvedExpenseLogs) => <ExpenseLogs expenseLogs={resolvedExpenseLogs} />}
  </Await>
</Suspense>

The key property now changes whenever we load the expense details page of another expense. This resets the Suspense boundary, which allows us to defer the new loader data.

Notice that we already do this in Chapter 6, Enhancing the User Experience, where we add a key property to our expense details Form component to reset the defaultValue on the input fields when navigating between expense details pages (p. 115).

The code for the book was fixed. You can find the change here: 1bbef0c

Before

The suspense fallback is only shown once when initially navigating to an expense details page.

Screen.Recording.2023-12-12.at.8.24.50.PM.mov

After

The suspense fallback is shown on every expense details page navigation.

Screen.Recording.2023-12-12.at.8.24.13.PM.mov

@andrelandgraf andrelandgraf added the with solution Tag for bug tickets that document the solution label Dec 13, 2023
@andrelandgraf andrelandgraf changed the title Deferring expense logs loader data does not work when navigating between expense detail pages. Deferring expense logs loader data not working when navigating expense detail pages. Dec 13, 2023
@nowakrr
Copy link

nowakrr commented Nov 1, 2024

It took me hours to make it work. Your suggestion was indeed very helpful, although for me it didn't work initially.
The reason was a reference to the loader data out of Suspense/Await tag.
It seems, Remix is checking, if there are any references to the deferred loader data, if so then it ignores defer at all.

Working example:

export default function Thread() {
  const loaderData = useLoaderData<typeof loader>();

  return (
    <>
      <Suspense fallback={<div>Loading..</div>} key={useParams().id}>
        <Await resolve={loaderData.model}>
          {(model) => <>
            <div>Loaded {model.id}</div>
          </>}
        </Await>
      </Suspense>
    </>
  )
}

Not working example:

export default function Thread() {
  const loaderData = useLoaderData<typeof loader>();

  return (
    <>
      <h1>{loaderData.model.name}</h1>
      <Suspense fallback={<div>Loading..</div>} key={useParams().id}>
        <Await resolve={loaderData.model}>
          {(model) => <>
            <div>Loaded {model.id}</div>
          </>}
        </Await>
      </Suspense>
    </>
  )
}

As you can see, there is a loader data reference within h1 tag which actually blocks whole route from being loaded in the defer/async style. It looks exactly as on your video.

It may look trivial, but when you have a really big component (or, like me - you're migrating existing components to Suspense/Await logic) and you overlook that, you may be surprised; or - if your loader resolves promises very fast - you may be even unaware that your defer logic doesn't work actually.

@andrelandgraf
Copy link
Collaborator Author

andrelandgraf commented Nov 2, 2024

Hey, @nowakrr! What version of Remix are you using, and what does your loader function implementation look like?

If model is of type Promise when returned from your loader function (using defer), then accessing loaderData.model.name outside of Suspense wouldn't work anyways, right?

@nowakrr
Copy link

nowakrr commented Nov 4, 2024

Hi @andrelandgraf yes, correct. It shouldn't work anyway. Although I think it's worth to mention that your solution DOES work, although, if it does not for some reason, make sure you guys don't have references to the deferred promises outside of Suspense tag. I was so focused on trying to make one single place with Suspense and fallback logic to work that I missed these blocking references in few other places, thinking it doesn't work.

Imagine having multiple nested components relaying on the same deferred promise. You need to take care that ALL references to the deferred promise across all nested components are wrapped into Suspense tag, otherwise it won't work on ANY of the components:

deferred promise (model) =>
└── Component A
        ├── Suspense (model)
        └── Component B (model as prop)
                ├── Suspense (model)
                └── Component C (model as prop)
                        └── Direct reference to model prop <- weak point

So: all references to the deferred promise must be wrapped inside Suspense tag, otherwise, none of Suspense tags will work, even though promise is deferred correctly in the loader.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working with solution Tag for bug tickets that document the solution
Development

No branches or pull requests

2 participants