Closed jcloutz closed 5 years ago
@jcloutz well, after all thanks for you contribution.- It's really well appreciated.
I will start answering your doubts:
You - Is it too much to use the container for everything? Should it only be used for resolving services, and repositories, etc..
Me - Maybe yes, maybe not. Depending on the case, using the container for all would be over engineer. For the case that this is a starter kit for project bootstrap, I would prefer to keep it simple, and only use it for Services and Repositories.
You - Interface organization: specification should they be in a single interface file? Or should they be broken up into their respective folders i.e. common, api/users, api/articles.
Me - I consider that they should be broken up in their respective folders.
You - Is the resolver redundant when using the container? It seems like it might make more sense to inject the repository directly into the controller since it's basically just proxying that functionality.
Me - Yes, it's redundant because know that you use container it's working as a proxy, like you said.
You - Would it make sense to use something like this Config to enable environment specific configs. It would probably help with testing.
Me - I would prefer relying on a more known solution, like the usage of .env files. Do you agree with that?
My ideas behind this project:
I would like to focus on set of basic features, and then, use this project as a template to develop some kind of cli like create-react-app
.
Well, that's all I have to say. I will review, and test a little bit better the code on my local machine, and sure this PR would be merged.
There are a few things that you should consider "broken" in this PR that I haven't had time to fix. Also some things that I learned through the process and after I made this PR by building a small app with it. At this point I have some mixed feelings on this PR to be honest.
I agree with you, using a .env and a object literal for config will provide a better developer experience with proper code completion
The change to absolute paths breaks static compilation
The way I broke up the container bindings into separate files is incorrect, this is the correct way to do it: https://github.com/inversify/InversifyJS/blob/master/wiki/container_modules.md
The interfaces could have been broken up into separate files then re-exported in index.ts
with export * from './repositories
. That's a moot point though if they are re-organized to live next to their implementations.
Using the container for everything really ratchets up the complexity. Each component is made up of an interface, concrete implementation, and a Symbol to call it in the container. I feel like some of the complexity it related to organization. Like you said the interfaces should be organized like their implementations. Should the symbols also live next to the interface they represent or should they be located centrally as a giant object literal?
Maybe just using container<Interface>get(key)
to fetch the controller would be better? That would let the controller use the @inject
decorator to get it's dependencies.
Tape is good for integration tests, but it lacks the functionality needed to do proper unit tests. Specifically it's missing spies which makes it very difficult to mock calls.
Closing this PR, since I want to keep a more functional approach on the code. Implementing IoC may introduce some complexity to those starting with node and Typescript.
Now that I have some free time after the holidays I wanted to take a crack at implementing dependency injection via Inversify as was discussed in #5. This is the first time that I've used it and any feedback would be welcome.
Things I'm not sure about:
Changes: