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

Handle failed serialized promises in createResource #2185

Closed
wants to merge 1 commit into from

Conversation

Brendonovich
Copy link

Summary

solidjs/solid-start#1520 provides a repro where a resource throws an error on the server, gets serialized during streaming, and then hydrates on the client with the status "ready" instead of "errored".
From what I can tell, this happens because resources that contain streamed promises don't handle the promise rejecting.
Notice how p.value isn't used anywhere for error handling.

if ("value" in p) {
  if ((p as any).status === "success") loadEnd(pr, p.value as T, undefined, lookup);
  else loadEnd(pr, undefined, undefined, lookup);
  return p;
}

To fix this I've added an extra branch that checks for status === "failure" and calls loadEnd with castError(p.value) as the third argument. This makes the repro work as expected.

How did you test this change?

Ran the repro with this PR's changes using pnpm link.

Copy link

changeset-bot bot commented Jun 9, 2024

⚠️ No Changeset found

Latest commit: b1e723b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@ryansolid
Copy link
Member

Ok, I will take a look. There was a reason why I didn't do this before, given the else case should just handle this I believe as there are only 2 outcomes. I wonder if I was missing casting. I thought it would be fine because Suspense/ErrorBoundaries would be fine, but I didn't account for the Resources own state.

@joprice
Copy link

joprice commented Jun 19, 2024

I tried out this commit in a new solid-start project. With the released version, I get a hydration mismatch. With this commit, I get the value stuck in 'pending' indefinitely. I'm pretty new to solid, so not sure if there's some other issue with this code.

 function Child() {
    let [data] = createResource(() => {
      if (import.meta.env.SSR) {
        console.log("loading on server")
        return Promise.reject(new Error("fail from server"))
      } else {
        console.log("loading on client")
        return Promise.resolve("success")
      }
    }, {
      deferStream: true
    })
    return (
      <div>
        <Switch>
          <Match when={data.state === 'ready'}>
            <div>data: {data()}</div>
          </Match >
          <Match when={data.state === 'errored'}>
            <div>error</div>
          </Match >
          <Match when={data.state === 'pending'}>
            <div>pending</div>
          </Match >
          <Match when={true}>
            <div>other</div>
          </Match>
        </Switch>
      </div>
    );
  }

  export default function Counter() {
    return (
      <ErrorBoundary fallback={function(ex, reload) {
        console.error("error boundary", ex)
        return (
          <div>
            <div>Failed loading: {ex.message}</div>
            <div>
              <button onClick={() => reload()}>Reload</button>
            </div>
          </div>)
      }}>
        <Child />
      </ErrorBoundary>
    );
  }

@ryansolid
Copy link
Member

@joprice you are guarding the resource read with the state check which prevents suspense from triggering which means that framework doesn't know it needs to wait/stream the response and it just returns early with pending. This is my mistake with the API design have state (people asked for it and I shouldn't have gave it). But that sounds like it is expected.

@ryansolid
Copy link
Member

Hey @Brendonovich thank you. In looking into it I realize the thing that caused me not to go this way was a red herring. It was due to some other weird hack that I'd done in the past. I've fixed it and in the process restored the code which has this fix in it. So I won't be merging this one but the fix will be in. Thank you very much for looking into this.

@ryansolid ryansolid closed this Jun 28, 2024
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.

3 participants