facebook / relay

Relay is a JavaScript framework for building data-driven React applications.
https://relay.dev
MIT License
18.28k stars 1.81k forks source link

Fragments render with missing data when using partial rendering #3651

Open janicduplessis opened 2 years ago

janicduplessis commented 2 years ago

When a query renders with partial data, but the network request fails the suspended fragments resolve with missing data instead of throwing the error.

Here's an example repro of the issue with the issue tracker example: https://github.com/janicduplessis/relay-examples/tree/partial-render-error-repro

To test, follow the instructions in the issue-tracker folder, then when the app is started click on one of the issue to open the issue details and see the error TypeError: undefined is not an object (evaluating 'data.comments.edges').

I investigated the cause and managed to patch the issue with this, if this seems like a good solution I can open a PR with the fix.

diff --git a/node_modules/react-relay/lib/relay-hooks/FragmentResource.js b/node_modules/react-relay/lib/relay-hooks/FragmentResource.js
index 180289e..1c82a2b 100644
--- a/node_modules/react-relay/lib/relay-hooks/FragmentResource.js
+++ b/node_modules/react-relay/lib/relay-hooks/FragmentResource.js
@@ -146,6 +145,10 @@ var FragmentResourceImpl = /*#__PURE__*/function () {
         throw cachedValue.promise;
       }

+      if (cachedValue.kind === 'error') {
+        throw cachedValue.error;
+      }
+
       if (cachedValue.kind === 'done' && cachedValue.result.snapshot) {
         this._reportMissingRequiredFieldsInSnapshot(cachedValue.result.snapshot);

@@ -401,7 +402,7 @@ var FragmentResourceImpl = /*#__PURE__*/function () {
     var promise = networkPromise.then(function () {
       _this5._cache["delete"](cacheKey);
     })["catch"](function (error) {
-      _this5._cache["delete"](cacheKey);
+      _this5._cache.set(cacheKey, {kind: 'error', error: error});
     }); // $FlowExpectedError[prop-missing] Expando to annotate Promises.

     promise.displayName = networkPromise.displayName;
maraisr commented 2 years ago

The fetch functions never threw an exception. Not sure fetch throws errors, only if dns or unlikely network stuff.

Can you link a more concrete example, and repro steps?

janicduplessis commented 2 years ago

The relay fetch function can throw, either with a network error (via fetch throwing) or if the api returns an error (https://github.com/janicduplessis/relay-examples/blob/partial-render-error-repro/issue-tracker/src/RelayEnvironment.js#L53). In the example repo I linked I simulate a network error for the IssueDetailRootQuery.

Here's the full repro steps:

janicduplessis commented 9 months ago

@captbaritone Sorry for the direct ping, but was wondering if you or someone at meta could have a look at this issue. More specifically how do you handle errors when using partial rendering as currently it seems to still render with missing data instead of having the fragment throw an error. I think my repro demonstrates the problem pretty well. I've been using the patch mentioned in the issue for a while and can commit some time to upstream it if it makes sense.