fix: invalid deferred data hanging response

[jacob-ebey]

Closes: #

• Docs
• Tests

Testing Strategy:

[changeset-bot]

🦋 Changeset detected

Latest commit: 4c2cfaa

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 18 packages

Name	Type
@remix-run/react	Patch
@remix-run/testing	Patch
create-remix	Patch
remix	Patch
@remix-run/architect	Patch
@remix-run/cloudflare	Patch
@remix-run/cloudflare-pages	Patch
@remix-run/cloudflare-workers	Patch
@remix-run/css-bundle	Patch
@remix-run/deno	Patch
@remix-run/dev	Patch
@remix-run/eslint-config	Patch
@remix-run/express	Patch
@remix-run/netlify	Patch
@remix-run/node	Patch
@remix-run/serve	Patch
@remix-run/server-runtime	Patch
@remix-run/vercel	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[benjaminsehl]

Thank you sir!

[brophdawg11]

I wonder if we could make this a TS error?

export function loader() {
  return defer({ 
    something: undefined,             // ❌
    lazy: Promise.resolve(undefined), // ❌

    else: null,                   // ✅
    lazy2: Promise.resolve(null), // ✅
  });
}

I didn't dig too deep but it looks like we have:

export type DeferFunction = <Data extends Record<string, unknown>>(
  data: Data, 
  init?: number | ResponseInit
) => TypedDeferredData<Data>;

I wonder if we could enhance that Record to allow something like NonNullable<unknown> | null | Promise<NonNullable<unknown> | null> as a value?

[jacob-ebey]

I'm honestly not sure how unknown would behave in this situation. @pcattori, what do you think?

[brophdawg11]

IIRC I got the NonNullable<unknown> | null trick from Pedro for use in RR a while back: https://github.com/remix-run/react-router/blob/main/packages/router/utils.ts#L160

I believe NonNullable<unknown> allows anything except undefined/null and then you can add null back in

[jacob-ebey]

Let's do that in a followup PR if we want to.

[brophdawg11]

Just clarifying - this should result in the warning and a resolution of null after the timeout? When I test this type of loader:

export function loader() {
  return defer({ data: Promise.resolve(undefined) });
}

It hangs for ~5 seconds then prints this in the console but does correctly resolve my <Await> with null:

Deferred data for routes/_index data resolved to undefined, defaulting to null.
Error: The render was aborted by the server without a reason.

I wasn't sure if this was supposed to still hit the timeout or it it should have flipped over and rendered null immediately?

However, if I introduce a delay I still get a Server timeout error boundary rendered - should that be defaulting over to null also?:

export function loader() {
  return defer({
    data: new Promise((r) => setTimeout(() => r(undefined), 1000)),
  });
}

[jacob-ebey]

Just clarifying - this should result in the warning and a resolution of null after the timeout? When I test this type of loader:

export function loader() {
  return defer({ data: Promise.resolve(undefined) });
}

It hangs for ~5 seconds then prints this in the console but does correctly resolve my <Await> with null:

Deferred data for routes/_index data resolved to undefined, defaulting to null.
Error: The render was aborted by the server without a reason.

I wasn't sure if this was supposed to still hit the timeout or it it should have flipped over and rendered null immediately?

However, if I introduce a delay I still get a Server timeout error boundary rendered - should that be defaulting over to null also?:

export function loader() {
  return defer({
    data: new Promise((r) => setTimeout(() => r(undefined), 1000)),
  });
}

That should flip over immediately. I'll see if I can get a test in for that.

[brophdawg11]

Alternate approach in remix-run/react-router#10690 - we'd still want to keep some of the improved error handling from here if we go with that.

[jacob-ebey]

Dependent on remix-run/react-router#10690

[github-actions]

🤖 Hello there,

We just published version v0.0.0-nightly-c8936ac-20230712 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[github-actions]

🤖 Hello there,

We just published version 1.19.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[github-actions]

🤖 Hello there,

We just published version 1.19.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!