solidjs / solid

A declarative, efficient, and flexible JavaScript library for building user interfaces.
https://solidjs.com
MIT License
31.64k stars 887 forks source link

Handle failed serialized promises in createResource #2185

Closed Brendonovich closed 4 days ago

Brendonovich commented 3 weeks ago

Summary

https://github.com/solidjs/solid-start/issues/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.

changeset-bot[bot] commented 3 weeks ago

⚠️ No Changeset found

Latest commit: b1e723b07a98eac720a9bf25ea71461dd92ab5f4

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 commented 2 weeks ago

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 commented 1 week ago

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 commented 5 days ago

@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 commented 4 days ago

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.