Replace useFragment with Apollo's native by fidelman · Pull Request #517 · microsoft/graphitation

fidelman · 2025-02-17T15:29:02Z

replace custom duct-tape useFragment implementation with Apollo's native useFragment https://www.apollographql.com/docs/react/data/fragments#usefragment
dep-up @apollo/client to 3.8.1 as useFragment is stable there

alloy · 2025-02-19T18:54:36Z

examples/apollo-watch-fragments/package.json

  "private": true,
  "dependencies": {
-    "@apollo/client": "~3.6.0",
+    "@apollo/client": "3.8.1",


Is this the version we are using in TMP today? If not, I believe we agreed to use TMP's version and include a patch-package.

nope, TMP uses 3.6.10. I gonna leave the graphitation as it is and patch it

because dep-up duct-tape's Apollo will cause problem in TMP as it is patched there. Discussed w/@vladar so far the easiest way is to patch duct-tape's Apollo (btw it is already it is), and only dep-up duct-tape in TMP.

alloy · 2025-02-19T18:56:59Z

packages/apollo-react-relay-duct-tape/src/storeObservation/compiledHooks/useCompiledFragment.ts

+      "the fragment. Did you forget to invoke the compiler?",
+  );

-  const client = useOverridenOrDefaultApolloClient();


This is missing from the new implementation, and I do recall that I added this for a reason. It would be good to check the history for when I added this ability and see if there's a reason left in those commits.

@alloy TLDR; we do not need that in the new implementation

My explanation

The old implementation were introduced by 3 commits:

bf2f03e - replace useState with observableQuery.getCurrentResult(), commit text: Only trigger useState change when we really want it to

21f61ee#diff-72832f4a1f9e169a840a8d60938b0a66249d0dc384d1cf3124f6ca049747f63f - replace forceUpdate with a dedicated hook, commit text: [apollo-react-relay-duct-tape] Implement useRefetchableFragment

87cb259, added a comment // Unclear why, but this yields twice with the same results, so skip one., commit text: [ARRDT] Cleanup

My assumption, correct me if you recall, The subscription was triggered twice (for unknown reason), so there is a logic to skip the first trigger and then forceUpdate triggers the hook again getting an actual value from the observableQuery.getCurrentResult();.

Here is how it looks without "skip". Just load and then the "Load more" button click

Here is how it looks w/ "skip" aka "old version":

.

And here is the logs of the new version:

As you can see it is identical to the old version, this is why I think we are good to remove that.

alloy · 2025-02-19T18:58:50Z

Great to see how simple this turned out to be! With this in place, and especially if we end up not needing to use TMP's patched Apollo version, I think we could actually go out and advertise duct-tape as something others could use. (But we'd need to discuss that more first.)

vladar · 2025-02-19T21:27:18Z

packages/apollo-react-relay-duct-tape/src/storeObservation/compiledHooks/useCompiledFragment.ts

+  invariant(
+    typeof from === "string",
+    "useFragment(): Expected the fragment reference to have a string id. " +
+      "Did you forget to invoke the compiler?",
+  );


I don't think we can do it here, because fragmentReference.id can be undefined, which is expected according to comments here:

fragmentReference.id is required for useFragment args to connect fragment with a query. @alloy do you think we can accept fragmentReference.id is nillable? Previous implementation did not have such invariant, but cache.watch did not require it either

vladar · 2025-02-19T23:00:11Z

packages/apollo-react-relay-duct-tape/src/storeObservation/compiledHooks/useCompiledFragment.ts

-  if (result.partial) {
-    invariant(
-      false,
-      "useFragment(): Missing data expected to be seeded by the execution query document: %s. Please check your type policies and possibleTypes configuration. If only subset of properties is missing you might need to configure merge functions for non-normalized types.",
-      JSON.stringify(
-        // we need the cast because queryInfo and lastDiff are private but very useful for debugging
-        (
-          observableQuery as unknown as {
-            queryInfo?: { lastDiff?: { diff?: Cache.DiffResult<unknown> } };
-          }
-        ).queryInfo?.lastDiff?.diff?.missing?.map((e) => e.path),
-      ),
-    );
-  }


We must keep this invariant, but trigger it if useFragment returns !result.complete

thanks, fixed

fidelman · 2025-02-21T15:28:22Z

Great to see how simple this turned out to be! With this in place, and especially if we end up not needing to use TMP's patched Apollo version, I think we could actually go out and advertise duct-tape as something others could use. (But we'd need to discuss that more first.)

sure it is!

vladar · 2025-02-25T12:57:11Z

yarn.lock

  dependencies:
    "@babel/highlight" "^7.18.6"

-"@babel/code-frame@^7.25.7":


I assume we can revert all changes to yarn.lock now that we have a patch?

fidelman requested review from alloy and vladar February 17, 2025 15:29

fidelman force-pushed the fidelman/replace-useFragment-with-apollos-native branch from e3b01ef to d27d4dd Compare February 17, 2025 15:39

fidelman marked this pull request as draft February 17, 2025 19:36

alloy reviewed Feb 19, 2025

View reviewed changes

vladar reviewed Feb 19, 2025

View reviewed changes

Andrei Fidelman added 2 commits February 24, 2025 17:54

Replace useFragment with Apollo's native

ef2edbf

Review fixes (path apollo and invariant result.complete)

6ee69a1

fidelman force-pushed the fidelman/replace-useFragment-with-apollos-native branch from a3f6a95 to 6ee69a1 Compare February 24, 2025 16:54

fidelman marked this pull request as ready for review February 24, 2025 16:55

fidelman requested review from alloy and vladar February 24, 2025 16:55

vladar reviewed Feb 25, 2025

View reviewed changes

Andrei Fidelman added 3 commits February 26, 2025 19:31

revert yarn lock

981a198

fix types in patch

06b44c5

Patch to fix missis useEffect in tests

002e242

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace useFragment with Apollo's native#517

Replace useFragment with Apollo's native#517
fidelman wants to merge 5 commits intomicrosoft:mainfrom
fidelman:fidelman/replace-useFragment-with-apollos-native

fidelman commented Feb 17, 2025

Uh oh!

alloy Feb 19, 2025

Uh oh!

fidelman Feb 21, 2025

Uh oh!

fidelman Feb 21, 2025

Uh oh!

alloy Feb 19, 2025

Uh oh!

fidelman Feb 24, 2025 •

edited

Loading

Uh oh!

alloy commented Feb 19, 2025

Uh oh!

vladar Feb 19, 2025

Uh oh!

fidelman Feb 24, 2025

Uh oh!

vladar Feb 19, 2025

Uh oh!

fidelman Feb 24, 2025

Uh oh!

fidelman commented Feb 21, 2025

Uh oh!

vladar Feb 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

fidelman commented Feb 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fidelman Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

My explanation

Uh oh!

alloy commented Feb 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fidelman commented Feb 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fidelman Feb 24, 2025 •

edited

Loading