dataloaden icon indicating copy to clipboard operation
dataloaden copied to clipboard

Replace code generation with generics

Open lwc opened this issue 2 years ago • 10 comments

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.

lwc avatar Apr 04 '22 07:04 lwc

code looks good to me...

  • added test on your pr. we dont need tests on the example anymore.
  • README.md needs updates too.
  • version update because its a breaking change

n33pm avatar Apr 05 '22 11:04 n33pm

code looks good to me...

  • added test on your pr. we dont need tests on the example anymore.
  • README.md needs updates too.
  • version update because its a breaking change

https://github.com/numero33/dataloaden

n33pm avatar Apr 11 '22 11:04 n33pm

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?

frederikhors avatar Apr 11 '22 12:04 frederikhors

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. don't get me wrong but I don't think it's optimal on vikstrous/dataloadgen if you cache the whole function.

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
		},
	})
cpu: Intel(R) Core(TM) i7-6920HQ CPU @ 2.90GHz
BenchmarkDataloadgen
BenchmarkDataloadgen/caches
BenchmarkDataloadgen/caches-8         	17150414	        65.49 ns/op
BenchmarkDataloadgen/random_spread
BenchmarkDataloadgen/random_spread-8  	 2686813	       649.7 ns/op
BenchmarkDataloadgen/concurently
BenchmarkDataloadgen/concurently-8    	   89816	     14618 ns/op
BenchmarkN33Dataloaden
BenchmarkN33Dataloaden/caches
BenchmarkN33Dataloaden/caches-8       	12214902	        92.20 ns/op
BenchmarkN33Dataloaden/random_spread
BenchmarkN33Dataloaden/random_spread-8         	 2403955	       532.4 ns/op
BenchmarkN33Dataloaden/concurently
BenchmarkN33Dataloaden/concurently-8           	   82574	     13577 ns/op

n33pm avatar Apr 11 '22 14:04 n33pm

@StevenACoffman can you operate on this repo?

Instead of a new version on this package, can we add this to gqlgen? Like gqlgen/dataloaden or gqlgen/dataloadgen?

We could also write in Readme that the project comes from this repo and new credits to @lwc.

frederikhors avatar Apr 11 '22 14:04 frederikhors

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.

StevenACoffman avatar Apr 11 '22 15:04 StevenACoffman

@lwc what do you think about creating a new repo since it’s all new?

frederikhors avatar Apr 11 '22 22:04 frederikhors

hello, I created a repository Warashi/dataloaden. which contains generics version of vektah/dataloaden. If you want to use generics version of dataloaden, please consider to use Warashi/dataloaden.

Warashi avatar Jul 25 '22 06:07 Warashi

@Warashi Have you compared your project to https://github.com/vikstrous/dataloadgen ? I wonder if you could collaborate.

StevenACoffman avatar Jul 25 '22 12:07 StevenACoffman

@StevenACoffman oh, I had missed dataloadgen. I would love to collaborate with you on this.

Warashi avatar Jul 26 '22 00:07 Warashi