New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Relay persisted queries and data preloading capabilities. #999

Merged
merged 30 commits into from Apr 9, 2018

Conversation

Projects
None yet
4 participants
@alloy
Copy link
Member

alloy commented Apr 6, 2018

Caching and preloading is working, but there’s still stuff to do. Will start keeping a list of TODOs.

  • On release, write out a ARGraphQLQueryMap.m file that hardcodes a dictionary that maps query names to IDs.
  • Make sure that on homepage reload the individual rails don’t still make individual requests
  • Caching on the homepage doesn’t appear to always work correct
  • Make MP accept persisted query IDs.

simulator screen shot - iphone 6 - 2018-04-06 at 17 21 57

#skip_new_tests

+ (NSArray<ARGraphQLQuery *> *)preloadQueriesWithArtistID:(NSString *)artistID;
{
return @[[[ARGraphQLQuery alloc] initWithQueryName:@"QueryRenderersArtistQuery" variables:[self propsWithArtistID:artistID]]];
}

This comment has been minimized.

@alloy

alloy Apr 6, 2018

Member

The idea is that VCs define a class method that returns the queries and their variables that should be used to preload those queries.

#endif

#ifdef HAVE_QUERY_MAP
#include "ARGraphQLQueryMap.m"

This comment has been minimized.

@alloy

alloy Apr 6, 2018

Member

Still need to do this.

@ArtsyOpenSource

This comment has been minimized.

Copy link

ArtsyOpenSource commented Apr 6, 2018

Warnings
⚠️

Missing Test Files:

  • src/lib/Components/Home/ArtistRails/__tests__/ArtistRail-tests.tsx
  • src/lib/Scenes/Favorites/Components/Artists/Relay/__tests__/FavoriteArtists-tests.tsx
  • src/lib/Scenes/Favorites/Components/Artworks/Relay/__tests__/FavoriteArtworks-tests.tsx
  • src/lib/Scenes/Favorites/Components/Categories/Relay/__tests__/FavoriteCategories-tests.tsx
  • src/lib/Scenes/Favorites/__tests__/index-tests.tsx
  • src/lib/Scenes/Home/Components/ForYou/Components/__tests__/ArtworkCarousel-tests.tsx
  • src/lib/Scenes/Home/Components/ForYou/__tests__/index-tests.tsx

If these files are supposed to not exist, please update your PR body to include "#skip_new_tests".

Generated by 🚫 dangerJS

NSDictionary *props = [self propsWithSelectedArtist:artistID tab:tab];
[queries addObject:[[ARGraphQLQuery alloc] initWithQueryName:queryName variables:props]];
}
return [queries copy];

This comment has been minimized.

@alloy

alloy Apr 6, 2018

Member

Here’s a more elaborate example of queries that can be preloaded.

}];
}

- (ARCellData *)jumpToGene
{
return [self tappableCellDataWithTitle:@"Gene" selection: ^{
id viewController = [[ARGeneComponentViewController alloc] initWithGeneID:@"website"];
NSString *geneID = @"minimalis";

This comment has been minimized.

@orta

orta Apr 6, 2018

Member

typo

This comment has been minimized.

@alloy

alloy Apr 6, 2018

Member

Good spot!

@orta

orta approved these changes Apr 6, 2018

Copy link
Member

orta left a comment

Clever, makes sense. Use the cache if available by running the query natively then checking the cache for that via a native module.

<edit> I've tried this locally, it is obscenely fast. </edit>

_gravityAPIHost = gravity.copy;
_metaphysicsAPIHost = metaphysics.copy;
_gravityURL = gravity.copy;
_metaphysicsURL = metaphysics.copy;
_userAgent = userAgent.copy;

This comment has been minimized.

@orta

orta Apr 6, 2018

Member

These were to be consistent with Eigen's terminology

}

return encodedCacheKey;
}

This comment has been minimized.

@orta

orta Apr 6, 2018

Member

Quite the keygen

forQueryID:(nonnull NSString *)queryID
withVariables:(nonnull NSDictionary *)variables;
{
[self setResponse:response forQueryID:queryID withVariables:variables ttl:0];

This comment has been minimized.

@orta

orta Apr 6, 2018

Member

zero feels weird here as the default TTL

This comment has been minimized.

@orta

orta Apr 6, 2018

Member

Ah, it uses ARGraphQLQueryCacheDefaultTTL

}
// async componentDidMount() {
// await this.refreshData()
// }

This comment has been minimized.

@alloy

alloy Apr 8, 2018

Member

should this stay? (orta)

alloy added some commits Mar 23, 2018

[fetchQuery] Update fetch function to work with document IDs.
In development mode we still POST the full query.
[QueryPreloader] Move cache out of RN module to work around issue.
Which is that the cache module was created twice. However, it looks like
that may just have been an error on my side, so I might revert this.

alloy added some commits Apr 8, 2018

@alloy alloy force-pushed the data-preloading branch from 9b27b2c to 4c37b56 Apr 8, 2018

@alloy alloy changed the title [WIP] Add Relay persisted queries and data preloading capabilities. Add Relay persisted queries and data preloading capabilities. Apr 8, 2018

@alloy

This comment has been minimized.

Copy link
Member

alloy commented Apr 9, 2018

@orta Alright, this is ready to go 👍

@alloy alloy referenced this pull request Apr 9, 2018

Closed

Persisted queries #2354

const { ARGraphQLQueryCache } = NativeModules

export function set(queryID: string, variables: object, response: string, ttl: number = 0): void {
ARGraphQLQueryCache._setResponseForQueryIDWithVariables(response, queryID, variables, ttl)

This comment has been minimized.

@damassi

damassi Apr 9, 2018

Member

Curious, why the _? Wouldn't this reaching in to the native side of things be considered public?

This comment has been minimized.

@orta

orta Apr 9, 2018

Member

I'd bet this is an indication of "you shouldn't use this" - we have to expose it, but that doesn't mean we have to let people consider it a public API for using inside RN components

Guess we shouldn't do this

This comment has been minimized.

@alloy

alloy Apr 9, 2018

Member

Yeah that native API is considered private and JS code should only use it through this JS module. However, tbh the original change was promoted because the native class needed to expose a public clearAll objc method as well that internally needed to dispatch to the internal GCD queue and that method would clash with the clearAll that gets exposed to the RN bridge.

@orta

orta approved these changes Apr 9, 2018

@@ -44,26 +47,26 @@ describe("fetchQuery", () => {

it("caches the fetched data", async () => {
await fetchQuery(operation, variables, cacheConfig)
expect(cacheMock.set).toHaveBeenCalledWith(operation.text, variables, response)
expect(cache.set).toHaveBeenCalledWith(operation.id, variables, JSON.stringify(response))
})

This comment has been minimized.

@orta

orta Apr 9, 2018

Member

Basically the response is now given as text, before it used to be object

- React/RCTImage (= 0.54.2)
- React/RCTLinkingIOS (= 0.54.2)
- React/RCTNetwork (= 0.54.2)
- React/RCTText (= 0.54.2)

This comment has been minimized.

@orta

orta Apr 9, 2018

Member

boo ;)

});
return queryMap[name];
}

This comment has been minimized.

@orta

orta Apr 9, 2018

Member

This code gets inlined

@orta

This comment has been minimized.

Copy link
Member

orta commented Apr 9, 2018

I'm gonna fix that RN version drop, and merge

@orta orta merged commit 902a29f into master Apr 9, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@orta orta deleted the data-preloading branch Apr 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment