-
Notifications
You must be signed in to change notification settings - Fork 517
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
Loading by an alternative key #2
Comments
The cache here is extremely simple, it's really just memoization. I think adding secondary key to the cache would be pretty complicating. At Facebook we actually have a two-phase load. vanity => userid first then userid => user object. There's two ways I can think of to do this: // follow-up in user code
var vanityLoader = new DataLoader(
vanities => promiseVanityToUserIDs(vanities)
)
var userLoader = new DataLoader(
ids => promiseUsersByID(ids)
)
var ide = await vanityLoader.load('ide').then(id => userLoader.load(id));
// or follow-up in loader code
var userLoader = new DataLoader(
ids => promiseUsersByID(ids)
)
var vanityLoader = new DataLoader(
vanities => promiseVanityToUserIDs(vanities).then(
ids => userLoader.loadMany(ids)
)
)
var ide = await vanityLoader.load('ide'); |
I wonder what the simplest possible approach would be to supporting a two-way cache? Maybe a cache population method? Or just providing a custom cache backend? What if you could write: var userLoader = new DataLoader(
ids => promiseUsersByID(ids).then(users => {
users.forEach(user => vanityLoader.prime(user.vanity, user));
return users;
})
)
var vanityLoader = new DataLoader(
vanities => promiseUsersByVanity(vanities).then(users => {
users.forEach(user => userLoader.prime(user.id, user));
return users;
})
)
// Possible race condition? What should happen?
var [ ide1, ide2 ] = await Promise.all(
vanityLoader.load('ide'),
userLoader.load(589436611)
)
console.log(ide1 === ide2) |
@leebyron thanks - I wrote up something like the example in your first comment last night and it's working fine. In my case it felt a little silly to have the second loader because the second key (e.g. vanity) was on the user object so I was reading from the same table in both loaders instead of doing just one query.
The thing I like about the custom cache backend is that you can add LRU policies and other behavior. One idea I was toying with is to keep per-user DataLoaders across requests for a few minutes so being able to control memory usage is important (this idea only works if you have a small number of servers or session stickiness though). Still leaning towards |
Most definitely. This occurred to me recently as well. A memoization cache is only acceptable for short-lived caches. You would definitely need something like an LRU cache if you wanted a longer-lived loader. |
This is now possible in v1.2.0. The race condition discussed here is more trivially and acceptably handled by making it a race between the synchronous calls to |
I imagine this is a well-understood problem in GraphQL-land: when you can query a user object by userid and vanity (both of which are fields of a User object), you still want just one user cache. Do you think it's worth complicating the DataLoader for this use case or just caching the vanity => userid mapping separately?
The text was updated successfully, but these errors were encountered: