-
Notifications
You must be signed in to change notification settings - Fork 79
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
Replace code generation with generics #61
base: master
Are you sure you want to change the base?
Conversation
dl.Prime(user.ID, &user) | ||
u := &user | ||
cpy := *u | ||
dl.Prime(user.ID, &cpy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the one breaking change, tests are updated to reflect new behaviour.
code looks good to me...
|
|
Hi guys, I would like to bring to your attention the following project: https://github.com/vikstrous/dataloadgen which has great benchmarks! What do you think? |
benchmarks are similar and on my hand i use concurrently far more often than cached values. dl := dataloaden.NewLoader(dataloaden.LoaderConfig[int, benchmarkUser]{
Wait: 500 * time.Nanosecond,
MaxBatch: 100,
Fetch: func(keys []int) ([]benchmarkUser, []error) {
users := make([]benchmarkUser, len(keys))
errors := make([]error, len(keys))
for i, key := range keys {
if key%100 == 1 {
errors[i] = fmt.Errorf("user not found")
} else if key%100 == 1 {
users[i] = benchmarkUser{}
} else {
users[i] = benchmarkUser{ID: strconv.Itoa(key), Name: "user " + strconv.Itoa(key)}
}
}
return users, errors
},
})
|
@StevenACoffman can you operate on this repo? Instead of a new version on this package, can we add this to gqlgen? Like We could also write in Readme that the project comes from this repo and new credits to @lwc. |
I do have access to merge pull requests in this repository, and I don't mind merging this PR if @lwc cannot. I don't think that this repo should be folded into gqlgen though. I would rather than the community coalesced and collaborated on a single dataloader implementation, I don't think we are quite there yet. Currently graph-gophers/graphql-go and 99designs/gqlgen can pick from any of the three mentioned dataloaders: At Khan Academy, we currently use (and prefer) graph-gophers/dataloader so I would personally rather not add vektah/dataloaden to the gqlgen repository. Also, we are stuck on Go 1.16 until GCP AppEngine supports a newer version, so Khan can't use generics yet. For those reasons, I don't think I can allocate time to maintain this repository until Khan Academy was actively using it. |
@lwc what do you think about creating a new repo since it’s all new? |
hello, I created a repository Warashi/dataloaden. which contains generics version of vektah/dataloaden. |
@Warashi Have you compared your project to https://github.com/vikstrous/dataloadgen ? I wonder if you could collaborate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loving this 😍
// Fetch is a method that provides the data for the loader | ||
Fetch func(keys []K) ([]T, []error) | ||
|
||
// Wait is how long wait before sending a batch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a typo here? i.e.
// Wait is how long wait before sending a batch | |
// Wait is how long to wait before sending a batch |
return l.LoadThunk(key)() | ||
} | ||
|
||
// LoadThunk returns a function that when called will block waiting for a User. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// LoadThunk returns a function that when called will block waiting for a User. | |
// LoadThunk returns a function that when called will block waiting for a T. |
return users, errors | ||
} | ||
|
||
// LoadAllThunk returns a function that when called will block waiting for a Users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// LoadAllThunk returns a function that when called will block waiting for a Users. | |
// LoadAllThunk returns a function that when called will block waiting for Ts. |
@StevenACoffman oh, I had missed dataloadgen. I would love to collaborate with you on this. |
Mostly done for my own edification.
Intentionally kept the API as similar as possible to demonstrate the existing test cases still pass (with one exception, noted).
Should keep the migration path very straightforward from generated to generic.