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

feat: add dependency injection #257

Merged

Conversation

TimoGlastra
Copy link
Contributor

@TimoGlastra TimoGlastra commented May 2, 2021

This was considerably easier than I thought it would be.

Just DI for now. No extension API yet.

  • Used TSyringe. Very lightweight (1 already used dependency)
  • Used symbols for interface based injectables. Maybe we want to use base classes later on to make it easier, but I wanted to keep most of the current working intact. Just replace it with DI
  • Use @scoped(Lifecycle.ContainerScoped) for injection. This is basically a singleton, however only for the current DI container. This means a child container won't share the same instance. Two reasons:
    • The tests were not happy with a singleton (could have bypassed this however)
    • This allows to run two agents side by side and don't have problems with the singletons
  • Instead of using Repository<RecordClass> we're now using RecordRepository. It was very hard to use this pattern. Maybe we can simplify it later on. For now it was easiest to just create a separate repository per record type.

Let me know what you think. I think this will make extension a lot easier moving forward.

Fixes #211

@TimoGlastra TimoGlastra requested a review from a team as a code owner May 2, 2021 22:44
@codecov-commenter
Copy link

codecov-commenter commented May 2, 2021

Codecov Report

Merging #257 (979d0a0) into main (2fef3aa) will increase coverage by 0.37%.
The diff coverage is 99.20%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #257      +/-   ##
==========================================
+ Coverage   89.01%   89.39%   +0.37%     
==========================================
  Files         179      190      +11     
  Lines        3422     3535     +113     
  Branches      403      404       +1     
==========================================
+ Hits         3046     3160     +114     
+ Misses        376      375       -1     
Impacted Files Coverage Δ
src/agent/Agent.ts 98.52% <96.29%> (-1.48%) ⬇️
src/agent/Dispatcher.ts 96.77% <100.00%> (+0.34%) ⬆️
src/agent/EnvelopeService.ts 100.00% <100.00%> (ø)
src/agent/MessageReceiver.ts 87.03% <100.00%> (+1.32%) ⬆️
src/agent/MessageSender.ts 93.33% <100.00%> (+1.02%) ⬆️
src/modules/basic-messages/BasicMessagesModule.ts 89.47% <100.00%> (+2.80%) ⬆️
src/modules/basic-messages/index.ts 100.00% <100.00%> (ø)
...asic-messages/repository/BasicMessageRepository.ts 100.00% <100.00%> (ø)
src/modules/basic-messages/repository/index.ts 100.00% <100.00%> (ø)
...les/basic-messages/services/BasicMessageService.ts 92.30% <100.00%> (+0.64%) ⬆️
... and 47 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fef3aa...979d0a0. Read the comment docs.

@TimoGlastra
Copy link
Contributor Author

Errors fixed, tests added

Copy link
Contributor

@JamesKEbert JamesKEbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Definitely moves us towards having extensibility capabilities.
To your singleton comments, this would provide the modules and such as singletons to only that Agent per say since we're using the scoped container, right? So it essentially means modules are singletons, except that agents have their own singletons (which I think make sense IMO).

@TimoGlastra
Copy link
Contributor Author

So it essentially means modules are singletons, except that agents have their own singletons (which I think make sense IMO).

Exactly!

Copy link
Contributor

@jakubkoci jakubkoci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Although it would be nice to avoid interface injection via Symbols, I would rather keep interfaces than creating classes where it shouldn't be a class (but maybe I'm too strict here 😄 )

I don't see much of a problem in changing Repository<RecordClass> to RecordRepository

@jakubkoci
Copy link
Contributor

  • This allows to run two agents side by side and don't have problems with the singletons

Yes, this is important to preserve 👍

@jakubkoci
Copy link
Contributor

I didn't merge it just for the case you want to merge it in some order with other PRs :)

@jakubkoci
Copy link
Contributor

One more thing. Perhaps it's not necessary but have you tested if it works with React Native? I know it should but TSyringe docs say something about Babel transform setup. It could be worthy to be sure it works and eventually describe what to do in our docs here. What do you think, @TimoGlastra ?

@TimoGlastra
Copy link
Contributor Author

Good point @jakubkoci. We need a way to easily test if the framework works in RN (not sure if bifold is already the easiest to to do that)

@TimoGlastra
Copy link
Contributor Author

TSyringe docs say something about Babel transform setup

This is only when using babel to do TS compilation. We use the native TS compiler and publish that to NPM so it should already include the TS metadata. But would be good to check

@JamesKEbert
Copy link
Contributor

Definitely a good point @jakubkoci! I think Bifold would be the easiest to do that--and I'd like to get it updated as necessary anyways. So, I could test this out later today if that'd be helpful.

@TimoGlastra
Copy link
Contributor Author

Definitely a good point @jakubkoci! I think Bifold would be the easiest to do that--and I'd like to get it updated as necessary anyways. So, I could test this out later today if that'd be helpful.

That would definitely be helpful @JamesKEbert! Thanks. It still needs to be updated to use the NPM package right? If we make that update it will be easier to quickly test new releases

@TimoGlastra TimoGlastra force-pushed the dependency-injection branch from e9a0313 to 979d0a0 Compare May 6, 2021 07:40
Copy link
Contributor

@JamesKEbert JamesKEbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great--and I have tested it and it is working as expected in a React Native context in Aries Bifold.

@TimoGlastra TimoGlastra force-pushed the dependency-injection branch from 979d0a0 to f1e79ab Compare May 7, 2021 07:09
@TimoGlastra TimoGlastra merged commit 1965bfe into openwallet-foundation:main May 7, 2021
@TimoGlastra TimoGlastra deleted the dependency-injection branch May 7, 2021 07:21
berendsliedrecht pushed a commit to berendsliedrecht/credo-ts that referenced this pull request May 11, 2021
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.

Improve dependency injection
4 participants