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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: migrate dataloaders sample to graph-gophers/dataloader #1871

Merged
merged 10 commits into from
Jan 31, 2022

Conversation

zenyui
Copy link
Contributor

@zenyui zenyui commented Jan 30, 2022

This PR replaces the dataloader sample with another using graph-gophers/dataloader.

NOTE: I had originally added this as a second example at the bottom of the old docs, but I found it made the dataloader docs too complex/long. FWIW, it seems dataloaden isn't properly maintained anymore, and IMO codegen is not necessary for this task (so best to avoid it). I'm happy to discuss this, though.

resolves #1714

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@coveralls
Copy link

coveralls commented Jan 30, 2022

Coverage Status

Coverage decreased (-0.01%) to 75.104% when pulling b89043e on zenyui:dataloader-sample into a30b68d on 99designs:master.

@frederikhors
Copy link
Collaborator

I think it must be clearly specified that this is an alternative to the other. And maybe we can do two pages, one for each.

Even if the other looks abandoned it could be taken over by the team that took over gqlgen.

Thanks for the wonderful job you have done, @zenyui.

@zenyui
Copy link
Contributor Author

zenyui commented Jan 30, 2022

I think it must be clearly specified that this is an alternative to the other. And maybe we can do two pages, one for each.

Even if the other looks abandoned it could be taken over by the team that took over gqlgen.

Thanks for the wonderful job you have done, @zenyui.

Do you think we should propose graph-gophers/dataloader as the preferred solution? Perhaps we could show the old docs as a "legacy" solution?

In my opinion, we should not be promoting the old library. This dataloader problem does not justify codegen, and go generics will eliminate that need completely, making the old library obsolete.

Thoughts?

@StevenACoffman
Copy link
Collaborator

StevenACoffman commented Jan 30, 2022

At Khan Academy, we are using graph-gophers/dataloader and fairly happy with it so we aren't really interested in maintaining the vektah/dataloaden. If someone else maintains vektah/dataloaden and adds some really great features, then we can always circle back at that point to the documentation and add back vektah/dataloaden as a choice.

I'm fine with merging this PR as is, but I'm also ok if you want to make additional adjustments.

@zenyui While you are updating documentation, we were contemplating commenting out the autobind in the exampl ein https://github.com/99designs/gqlgen/blob/master/docs/content/config.md per #1860 since when the models directory is empty, it causes an error that is confusing people.

If you get a chance to run through that example without autobind and make sure it still works, I would love to merge that change as well before the next release.

docs/content/reference/dataloaders.md Outdated Show resolved Hide resolved
docs/content/reference/dataloaders.md Show resolved Hide resolved
docs/content/reference/dataloaders.md Outdated Show resolved Hide resolved
@zenyui
Copy link
Contributor Author

zenyui commented Jan 30, 2022

Alright, appreciate the comments @frederikhors and @StevenACoffman .

At this time, I vote we merge the PR as is and revisit in the future if dataloaden adds new maintainers / features.

@frederikhors
Copy link
Collaborator

You convinced me. Also I am using https://github.com/graph-gophers/dataloader successfully.

For me it can be merged.

For the autobind I have already created #1872, @StevenACoffman.

@StevenACoffman StevenACoffman merged commit 06bbca3 into 99designs:master Jan 31, 2022
@zenyui zenyui deleted the dataloader-sample branch January 31, 2022 15:46
@zenyui
Copy link
Contributor Author

zenyui commented Jan 31, 2022

btw @StevenACoffman seems the github actions workflow broke on an integration test. can you try re-running it?

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

Successfully merging this pull request may close these issues.

Add another dataloader project and example to docs page
4 participants