Change type of keys to []interface{}

[cloudlena]

Why does keys have to be a []string? I am working with https://labix.org/mgo where all IDs are of type bson.ObjectId. This makes using dataloader somewhat impractical because I have to convert all bson.ObjectIds into strings before piping them into the Load function of dataloader. In there, I have to convert them back to bson.ObjectIds to execute the MongoDB queries. If the Load function would except the keys as []interface{} instead of []string, that would make my life a lot easier because I could directly pipe the bson.ObjectIds into it.

The context.Context package works the same way. There, a key can also be interface{}.

[cloudlena]

I just saw that this was already discussed in #23. Sorry for the duplicate! Nevertheless, it gives no reason for being closed and I saw that they had similar discussions on https://github.com/facebook/dataloader/pull/15 so I thought I'd bring it up again.

[tonyghita]

I closed the issue because I decided to just use strings rather than force users to use interface{}.

If a similar change was merged into github.com/facebook/dataloader we should probably consider something similar here.

[nicksrandall]

Thanks for bringing this up @mastertinner. It appears that the spec has changed since I built this library so I'm open to this change. I'll work on it tomorrow and release a new version.

[nicksrandall]

@mastertinner @tonyghita What do you think of this: https://github.com/nicksrandall/dataloader/compare/v4

If you are ok with the change, I'll make the v4 branch the main branch.

[tonyghita]

@nicksrandall thanks! I think that looks good :) Would it be possible to start tagging releases? It works a bit nicer with https://github.com/golang/dep

[cloudlena]

Absolutely fantastic! Thanks so much for implementing this so quickly, @nicksrandall!

[nicksrandall]

This change has been merged in. @tonyghita I also tagged the release.

[cloudlena]

Thanks, @tonyghita and @nicksrandall! You guys have been awesome with replying to my requests!

[tonyghita]

Just wanted to share my experience upgrading from v2 to v4.1.0 in my example project.

https://github.com/tonyghita/graphql-go-example/commit/c0369fd0bf7a2832feff53e672b58e43a7c4c642

After the upgrade, there is a lot more copying (due to []interface{}), but I think it should be cheap enough to be acceptable.

[nicksrandall]

@tonyghita I’m also worried about this as I have had a similar experience. What if instead of allowing empty interface, we required a specific interface. Something like:

type Key interface{
  String() string
  Value() interface{}
}

Do you think that would help?

[tonyghita]

Hmm.... I'm not exactly sure what the best path forward looks like yet.

In my work use-cases, we have way more dataloaders with string keys than those with composite keys. For now, we just create composite string keys with some kind of separator like ; for those loaders. This is a pretty brittle approach but generally it works because these cases are fairly limited.

I'll play around with some ideas and come back to this. In the meantime it might help if we write some benchmarks... I'll look into this (will help catch any perf regressions).