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

Initial implementation of DI on handler package #76

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fernandez14
Copy link

  • Added facebookgo/inject on handler package
  • Posts module

- Added facebookgo/inject on handler package
- Posts module
@bentranter
Copy link
Collaborator

Hey @fernandez14, this is a really interesting PR, but I'm not sure if it'd fit within this project for a couple of reasons. Of course it's up to @dinever to make the final call, but I just wanted to share my thoughts:

  • Do we really need Gorm? We're already using Meddler, and I feel like using two ORMs is going to get very confusing.
  • What problem is adding dependency injection solving? I understand we've run into a few small cyclical dependency issues, but that hasn't been a big deal yet. I'm very weary of dependency injection in Go; I think it adds unnecessary complexity, I don't think it's idiomatic Go, which would make this code harder for new contributors to pick up and contribute to if they first have to wrap their head around the concept of dependency injection. I get that it helps eliminate globals, but so far I think this project has been really good about splitting things into packages appropriately in order to avoid having too many globals.

I know I'm coming off as really negative here, but I think Dingo is as close to the perfect project for new Gophers to use, hack on, and contribute to as you can get. I'd hate for us to do anything that makes that less true, and I'm worried adding another ORM and dependency injection might do that. On the other hand, I'd never even considered the dependency injection approach, so this has taught me a lot about what other Go projects are doing, so for that alone, thank you @fernandez14, and thanks for the contribution 😄

@samdfonseca
Copy link
Collaborator

@fernandez14 im on the same boat as @bentranter regarding this pr. i dont see a particularly strong reason to introduce gorm or facebookgo/inject. that said, im a big fan of dependency injection, but i'd rather see it done with mocking.

@fernandez14
Copy link
Author

fernandez14 commented May 24, 2016

@bentranter @samdfonseca
I completely understand your point of view here but I believe you are not seeing the long term advantages of both DI and gorm and here's why:

  • On the first place you are talking about Meddler, I don't see how writing pure SQL for simple selects could be easier to maintain when using more sql databases while Gorm makes a really good job abstracting the differences between them (the more flexible the better). What about release upgrades? migrations abstracted too! and in my honest opinion I don´t see why this could be in any way more confusing
    db.AutoMigrate(&User{})
    rather than writing an sql file with differences between drivers and filling code with idiomatic conditionals.
  • Code API will eventually become much more complex when adding more possible services or endpoints or stuff to do (fine-grained roles, better dashboards or plugins or anything.), once again, the more flexible the better. Golang provides us with much better ways to do stuff rather than writing a bunch of functions and global vars to make it idiomatic, there's embedding, interfaces, receivers and I don't see here magic DI because where not skipping the strict typing with reflection. I don´t even wanna say how that goes with keeping thing DRY and readable.

Also keep in mind that its a tool made by the good guys! Facebook does pretty good contributions to Golang.

At the end its completely opt to you guys, I liked the project and I tried to make my contribution just let me know to update my PR.

@samdfonseca
Copy link
Collaborator

@fernandez14 if the plan is to replace meddler with gorm, im all for that. my concern was ending up using gorm in some places and meddler in others. i'll also throw qb out there as a third option. i haven't used it yet but it looks like it provides better abstraction than meddler and isn't as invasive as gorm.

i agree that we need to move away from globals and can make better use of things like interfaces and embedding to help manage complexity as the apis evolve. that said, i dont think we're at the point were we need to resort to a reflection-heavy DI approach. there's nothing wrong with the approach taken here, its just a bit over-engineered. we should have no problem keeping things under control if we make better use of go's support for object composition.

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.

None yet

3 participants