Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is using this kind of cacheKeyFn bad practice? #75

Closed
kolpav opened this issue Mar 25, 2017 · 9 comments
Closed

Is using this kind of cacheKeyFn bad practice? #75

kolpav opened this issue Mar 25, 2017 · 9 comments

Comments

@kolpav
Copy link

kolpav commented Mar 25, 2017

Hello,

First of all thank you for this cool library my db server is no longer on fire 馃槃

Before I jump to my question I would like to explain in detail problem I am trying to solve. Maybe I am doing it wrong alltogether but you can skip to question at the end if you like.

Explanation

I am using relay style pagination it could be described by these interfaces

interface IEntity {
  id: String
}
interface IEdge {
  node: IEntity
  cursor: String
}
interface IPageInfo {
  hasNextPage: Boolean
  startCursor: String
  endCursor: String
  orderBy: String
}
interface IPaginated {
  edges: [IEdge]
  pageInfo: IPageInfo
}

And I have a query which returns posts based on count, after, orderBy like so:

posts(after: String, count: Int!, orderBy: PostOrderFn): IPaginated

Everything works perfectly but if you want to resolve value for hasNextPage from IPageInfo you need to make same request to db as you would for edges but with count + 1 and see if hasNextPage = results.length > count. Hope that makes sense.

I didn't liked that very much. We are making quite expensive query just to know if there are any more results. I solved this by using postModel.getPosts.load({ after, orderBy, count: count + 1 }) for edges and just droping last result if there were more than count. Now I can make same request as I made for edges for hasNextPage it looks completelly same postModel.getPosts.load({ after, orderBy, count: count + 1 }) so I get cached version, bypassing call to db alltogether! But I need to use this ugly cacheKeyFn

const objectCacheKeyFn = key => {
  if (isObjectLike(key)) {
    return JSON.stringify(Object.keys(key).sort().reduce((acc, val) => {
      acc[val] = key[val]
      return acc
    }, {}))
  } else {
    return key
  }
}

This question can be applied to any call to load where its argument is an object and not just simple id.
Is this wrong?

Question

I basically need this:

...
var postLoader = new DataLoader(...)
var promiseA = postLoader.load({ after, orderBy, count })
var promiseB = postLoader.load({ after, orderBy, count })
assert(promiseA === promiseB)
@leebyron
Copy link
Contributor

You definitely need a cacheKeyFn if you're using an object as a key where you want to treat that object as a value rather than a reference. I like the one you made that considers keys being defined in a different order as equivalent, very good.

@tuananh
Copy link

tuananh commented Oct 6, 2018

@leebyron if we do like this, how can we make use of, say where in of mysql (knex)?

@timscott
Copy link

timscott commented Apr 3, 2019

@kolpav Is this still the best way to do this? Is there a better (official) way to pass GraphQL args down to dataloader functions?

@kolpav
Copy link
Author

kolpav commented Apr 9, 2019

@timscott You need to be able to turn your args to value which can be used as an object key (string or symbol). In my case I am turning object into string while ignoring order of keys because {id: '1', orderBy: 'new'} produces same query as {orderBy: 'new', id: '1'} so I consider them equivalent. It was written for my specific use case and it worked fine your needs might differ so I don't think there should be official cacheFn.

@timscott
Copy link

timscott commented Apr 9, 2019

@kolpav Yeah, I ended up adding the args to the key and doing just JSON.stringify(key). The code sample in your OP does a shallow stringify, which might miss differences in deep objects like InputTypes.

That's a great point about orderBy and other args that might not affect the actual content. I guess those have to me pulled out by name, which seems might not scale well. Perhaps InputTypes or Interfaces could help matters.

@kolpav
Copy link
Author

kolpav commented Apr 11, 2019

@timscott
I don't think you should use JSON.stringify(key) because it does not guarantee key order it could produce some really hard to catch bugs.

The code sample in your OP does a shallow stringify, which might miss differences in deep objects like InputTypes.

Entire object is stringified just top levels keys are ordered but I guess thats what you meant. You could write recursive version of my objectCacheKeyFn but I would think twice before caching resolvers with such complicated arguments that you need nested objects. Are you sure there is a chance of calling same resolver with same arguments more than once during lifetime of a single request?

@timscott
Copy link

@kolpav Ah, good point. Maybe something like this covers every scenario: https://github.com/puleos/object-hash

2 similar comments
@timscott
Copy link

@kolpav Ah, good point. Maybe something like this covers every scenario: https://github.com/puleos/object-hash

@timscott
Copy link

@kolpav Ah, good point. Maybe something like this covers every scenario: https://github.com/puleos/object-hash

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants