feat: support metaphysics CDN and support cacheable #10842

MounirDhahri · 2024-09-25T12:10:01Z

Description

This PR makes the following changes:

Add MP CDN support to Eigen
Support our custom Graphql @cacheable directive
Both changes are hidden behind a feature flag

Open questions:

Currently I am deleting the access token for cacheable requests. I worry though that a query might grow into becoming non-cacheable (for example after adding Me) and the request will fail and there is nothing we can do to solve it from the CDN level. A retry strategy here seems like a requirement to me here 🤷
useLazyLoadQuery sets force to true always by default. Should we mimic that for network queries like in Force?
I noticed that query persistence logic in Eigen was "broken" - it always led to an additional request. Does it make sense to keep it? what measurable performance improvement does it bring?

PR Checklist

I have tested my changes on iOS and Android.
I hid my changes behind a feature flag, or they don't need one.
I have included screenshots or videos, or I have not changed the UI.
I have added tests, or my changes don't require any.
I added an app state migration, or my changes do not require one.
I have documented any follow-up work that this PR will require, or it does not require any.
I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

I would like at least one of the reviewers to run this PR on the simulator or device.

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

support metaphysics CDN and support cacheable - mounir

Need help with something? Have a look at our docs, or get in touch with us.

mzikherman · 2024-09-25T12:48:02Z

Currently I am deleting the access token for cacheable requests. I worry though that a query might grow into becoming non-cacheable (for example after adding Me) and the request will fail and there is nothing we can do to solve it from the CDN level. A retry strategy here seems like a requirement to me here

If so - wouldn't a dev notice in development that their query doesn't work? As in, they have a cacheable query. Then, they add a fragment that depends on access token (like something scoped under me). Once they do that...that query wouldn't return the right data, the access token would have been removed and so their personalized fragment they added wouldn't resolve. I feel like the dev would notice. (that being said we can think about tooling/validations to improve dev workflow).

MounirDhahri · 2024-09-25T13:03:38Z

That's actually a good point @mzikherman - I can add some dev tooling to make sure that the error thrown shows a hint about @refetchable ;)

brainbicycle · 2024-09-25T14:07:38Z

src/app/system/relay/middlewares/metaphysicsMiddleware.ts

-    }
+    body = { query: require("../../../../../data/complete.queryMap.json")[queryID], variables }
+    req.fetchOpts.body = JSON.stringify(body)
+    return await next(req)


are we no longer using the persisted queries?, oh just saw your comment, I think it works when the query map in eigen is in line with MP? I could be wrong but think it works some time. I am not sure about how beneficial it actually is though, it does annoy me when trying to debug but could be a consequential change we should probably test.

I am 100% in favor of removing this for the time being 👍 I feel like our concern for low powered devices is unwarranted given our demographic, and in any case, is trivial.

This also vastly improves the developer experience, sentry experience, and so on and so forth. Its worth it.

i am not opposed to it but I don't want us to just assume it will have no effect, the app is chatty and the bandwidth might add up, I think at least we should try to understand what the impact might be and monitor.

Good point on Eigen being chatty @brainbicycle - Let me see if I can keep this then we can remove it in a separate PR. Here I deleted it because:

I wanted to reproduce the experience on a production device as close as possible

I wanted some to evaluate the improvement in latency

I am not sure what will be the synergy like between the CDN and MP using the persisted queries 🤷 but I guess it will work out of the box 👀

src/app/system/relay/middlewares/metaphysicsMiddleware.ts

src/app/system/relay/middlewares/cacheHeaderMiddleware.ts

MounirDhahri · 2024-09-25T15:59:04Z

Interesting results with the CDN

Query name	MP	CDN -> MP	CDN
HomeViewQuery	335ms	Cell	320ms-370ms
HomeViewSectionActivity	415ms	475ms	X
HomeViewSectionArtworks	452ms	515ms	X
HomeViewSectionHeroUnits	256m	X	113ms
HomeViewSectionGalleries	248ms	X	274ms

Note: these are values from staging, I assume in production the impact will be greater!

MounirDhahri · 2024-09-25T16:04:44Z

src/app/routes.ts

+}
+
+// Helper method that returns the current URL
+export const getCurrentURL = () => {


I am glad I managed to use this helper that I spent hours to write at the beginning of the year

Not necessarily for this PR, but it would be great to separate this stuff out. In force, etc, we have a

src/system/router

folder, and then inside that we should store utils related to the router

src/system/router/utils

Then all of the implementation details for our router can go in there, and our routes file can remain clear and behavioral.

good idea - that should be fast to do. I will do it

ArtsyOpenSource · 2024-09-25T16:05:55Z

This PR contains the following changes:

Dev changes (support metaphysics CDN and support cacheable - mounir)

Generated by 🚫 dangerJS against b1db98b

damassi · 2024-09-25T17:46:24Z

src/app/Scenes/Articles/Articles.tsx

@@ -13,7 +13,11 @@ import { useTracking } from "react-tracking"
 import { ArticlesList, ArticlesPlaceholder } from "./ArticlesList"

 export const Articles: React.FC = () => {
-  const queryData = useLazyLoadQuery<ArticlesQuery>(ArticlesScreenQuery, articlesQueryVariables)
+  const queryData = useLazyLoadQuery<ArticlesQuery>(ArticlesScreenQuery, articlesQueryVariables, {


What we've done in Folio is define a useSystemQueryLoader that encompasses this kind of default behavior, and then leaned on that: https://github.com/artsy/energy/blob/main/src/system/relay/useSystemQueryLoader.tsx.

As we're changing global behavior, we should hide as much implementation detail stuff as possible behind an artsy-specific API, and then evangelize that 👍

Thanks for your feedback. Sorry for not answering this earlier. I think your comment here makes total sense. I was wondering why you went that direction in Force and in Folio but I think that's more future proof especially in cases like this where we want to update the behaviour in multiple screens.
This actually reminded me of this talk that I watched recently where the speaker is raising a good point about using your own hooks around state library / in our case this applies to relay hooks perfectly https://youtu.be/KI9xTG6wpeY?si=DR8Szsu3cr6EGnQm&t=379

damassi · 2024-09-25T17:49:27Z

src/app/system/relay/middlewares/metaphysicsMiddleware.ts

+        ? unsafe__getEnvironment().metaphysicsCDNURL
+        : unsafe__getEnvironment().metaphysicsURL
+
+      return metaphysicsURL


damassi

Code looks good @MounirDhahri ! thanks for diving into this. One suggestion about using a useSystemQueryLoader hook and then updating various sections using that. From there we can continue building out a more specific API for relay operations in the app, from there.

MounirDhahri requested review from damassi, gkartalis and brainbicycle September 25, 2024 12:10

MounirDhahri self-assigned this Sep 25, 2024

MounirDhahri force-pushed the feat/support-cacheable-in-eigen branch 2 times, most recently from 0ba89d2 to b7af2c9 Compare September 25, 2024 13:40

brainbicycle reviewed Sep 25, 2024

View reviewed changes

joeyAghion reviewed Sep 25, 2024

View reviewed changes

src/app/system/relay/middlewares/metaphysicsMiddleware.ts Outdated Show resolved Hide resolved

src/app/system/relay/middlewares/cacheHeaderMiddleware.ts Outdated Show resolved Hide resolved

MounirDhahri force-pushed the feat/support-cacheable-in-eigen branch from 9d7b202 to 388a95d Compare September 25, 2024 15:31

MounirDhahri marked this pull request as ready for review September 25, 2024 16:03

MounirDhahri commented Sep 25, 2024

View reviewed changes

damassi reviewed Sep 25, 2024

View reviewed changes

damassi previously approved these changes Sep 25, 2024

View reviewed changes

MounirDhahri dismissed damassi’s stale review via cfb9b86 September 26, 2024 07:10

MounirDhahri added 7 commits September 26, 2024 09:25

feat: support metaphysics CDN and support cacheable

a04a2c6

chore: strip access token from cacheable requests

22983a9

chore: remove persistence tests

36de8ec

chore: address review comments

fad502d

chore: skip CDN for force.true requests

e6beb18

chore: remove unnecessary headers

690393a

feat: bring back persisted queries

b1db98b

MounirDhahri force-pushed the feat/support-cacheable-in-eigen branch from cfb9b86 to b1db98b Compare September 26, 2024 07:26

MounirDhahri merged commit a4db61f into main Sep 26, 2024
7 checks passed

MounirDhahri deleted the feat/support-cacheable-in-eigen branch September 26, 2024 07:53

MounirDhahri mentioned this pull request Sep 26, 2024

chore: better DX around for @cacheable #10847

Merged

8 tasks

This was referenced Sep 26, 2024

feat: simplify relay caching strategy #10848

Merged

chore: better folders for router [WIP] #10853

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support metaphysics CDN and support cacheable #10842

feat: support metaphysics CDN and support cacheable #10842

MounirDhahri commented Sep 25, 2024

mzikherman commented Sep 25, 2024

MounirDhahri commented Sep 25, 2024

brainbicycle Sep 25, 2024 •

edited

Loading

damassi Sep 25, 2024 •

edited

Loading

damassi Sep 25, 2024

brainbicycle Sep 25, 2024

MounirDhahri Sep 26, 2024

MounirDhahri commented Sep 25, 2024 •

edited

Loading

MounirDhahri Sep 25, 2024

damassi Sep 25, 2024

MounirDhahri Sep 26, 2024

ArtsyOpenSource commented Sep 25, 2024 •

edited

Loading

damassi Sep 25, 2024

MounirDhahri Sep 26, 2024

damassi Sep 25, 2024

damassi left a comment

feat: support metaphysics CDN and support cacheable #10842

feat: support metaphysics CDN and support cacheable #10842

Conversation

MounirDhahri commented Sep 25, 2024

Description

PR Checklist

To the reviewers 👀

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

mzikherman commented Sep 25, 2024

MounirDhahri commented Sep 25, 2024

brainbicycle Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

damassi Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MounirDhahri commented Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArtsyOpenSource commented Sep 25, 2024 • edited Loading

This PR contains the following changes:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damassi left a comment

Choose a reason for hiding this comment

brainbicycle Sep 25, 2024 •

edited

Loading

damassi Sep 25, 2024 •

edited

Loading

MounirDhahri commented Sep 25, 2024 •

edited

Loading

ArtsyOpenSource commented Sep 25, 2024 •

edited

Loading