chore: better DX around for @cacheable #10847
Conversation
MounirDhahri
requested review from
joeyAghion,
damassi,
mzikherman and
araujobarret
| }) | ||
| } | ||
|
|
||
| return encodeURI(`${webURL}${path}`) | ||
| const queryParamsString = queryParams.map((params) => |
There was a problem hiding this comment.
the query params where not present in the helper so I added them here
| @@ -16,18 +17,34 @@ export const shouldSkipCDNCache = (req: GraphQLRequest) => { | |||
| // - a known personalized argument is present in the query (for example `include_artworks_by_followed_artists`) | |||
| // - nocache param is present in the url | |||
|
|
|||
| if (req.cacheConfig?.force === true) { | |||
There was a problem hiding this comment.
I moved this down since it will be covered by the default case anyway
| if (isRequestCacheable(req)) { | ||
| if (req.cacheConfig?.force === true) { | ||
| if (__DEV__) { | ||
| console.warn( |
There was a problem hiding this comment.
can we use info here? These warnings in the simulators/emulators are pretty annoying
There was a problem hiding this comment.
I tried both actually but they both seems equally ugly 🥲 except that the warn shows the count better
Screen.Recording.2024-09-26.at.10.38.46.mov
There was a problem hiding this comment.
😢 then I'd vote only to console.log, these bubbles are bad for DX because they block the screen and all the time we have to close them
There was a problem hiding this comment.
I agree that console.warns can be annoying, but that's only if you can't dismiss them or don't know why they're occuring. Here, the action item should be clear, for example you need to remove @cacheable. I do though think it's bad DX to declare your query as cacheable, expecting it to be cacheable, but it's not cacheable because something changed in the arguments and you aren't aware of it
There was a problem hiding this comment.
yeah, after a 2nd thought I'm fine with it, it should really pester someone in this case, 👍 for the .warn then
| @@ -19,11 +19,11 @@ export const hasNoCacheParamPresent = (url: string) => { | |||
| return false | |||
| } | |||
|
|
|||
| export const SKIP_CACHE_ARGUMENTS = ["includeArtworksByFollowedArtists", "collectionID"] | |||
There was a problem hiding this comment.
isn't better to add some comments around it to give engineers direction and/or maybe add it to some docs about cache in Eigen?
There was a problem hiding this comment.
that's a valid point - we need something like this but for Eigen. I will draft something
Description
This PR makes a few adjustments to follow up on
@cacheablePR. The goal here is to make it clear to developers when@cacheableis not going to take effect@cacheableto a query whereforceis set totrue@cacheableto a query wherenocacheis present in the URL@cacheableto a query where a specialized argument is presentPR Checklist
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
@cacheable- mounirNeed help with something? Have a look at our docs, or get in touch with us.