-
Notifications
You must be signed in to change notification settings - Fork 204
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
feat: add dependency injection #257
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Errors fixed, tests added |
There was a problem hiding this 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).
Exactly! |
There was a problem hiding this 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
Yes, this is important to preserve 👍 |
I didn't merge it just for the case you want to merge it in some order with other PRs :) |
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 ? |
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) |
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 |
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 |
e9a0313
to
979d0a0
Compare
There was a problem hiding this 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.
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
979d0a0
to
f1e79ab
Compare
This was considerably easier than I thought it would be.
Just DI for now. No extension API yet.
@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:Repository<RecordClass>
we're now usingRecordRepository
. 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