search

samber / do

โš™๏ธ A dependency injection toolkit based on Go 1.18+ Generics.
https://pkg.go.dev/github.com/samber/do
MIT License
1.71k stars 67 forks source link

[๐Ÿ‘ท] Road to v2 ๐Ÿš€ #45

Open samber opened 9 months ago

samber commented 9 months ago

Hi there ๐Ÿ‘‹,

Over the past 18 months, many of you reported bugs or provided valuable feedback on various aspects of this project, such as error handling, debugging, invocation API, documentation, and more... Thanks for your contributions! ๐Ÿ™Œ

Work on this library has provided me with significant learning opportunities and now I believe it's time to advance this project further.

Most of the work for v2 has been done already. I'm targeting the end of January for a release. It will depend on additional requirements.

go get -u github.com/samber/do/v2@v2.0.0-beta.7

Scope

This milestone will lead to major improvement: Scope.

A Scope was previously referred to as an Injector and can be viewed as a module of an application, with restricted visibility. I advocate for each project possessing a RootScope for common code, and shifting business logic to dedicated Scopes.

The Scopes are structured with a parent (RootScope or other Scope), numerous Services, and potentially a few children Scopes as well.

RootScope owns options and is the only Scope that supports cloning.

Services from a Scope can invoke Services available locally or in parent Scopes. Therefore, a single Service can be instantiated many times in different branches of the Scope tree, without conflict.

Scope will be almost invisible to developers: services will keep using Injector API without awareness of the underlying implementation, whether it's a RootScope, Scope or VirtualScope.

A chain of Service invocation will instantiate many VirtualScope, in order to track dependency cycles.

A debugging toolchain has been added to illustrate the service dependency chain and Scope tree.

Checklist

The library has been almost entirely recoded, but most of do@v1 API has been preserved (see breaking changes below).

To be challenged

More existing issues might be added to the v2 milestone.

Documentation

Testing

Code coverage has been improved compared to v1. ๐Ÿ˜

Coverage:

Breaking changes

If you consider the API must be improved, please write a comment on this issue. No breaking change will be done in v2 minor releases. And I hope v2 is going to be the last major version.

samber commented 9 months ago

Debugging

Here is the output of do.ExplainService[*MyService](Injector) ๐Ÿ‘‡

Scope ID: scope-id-123
Scope name: scope-child
Service: SERVICE-E
Service type: lazy
Service build time: 1s
Invoked: /home/samber/project/github.com/samber/do/di_explain_test.go:fakeProvider5:38

Dependencies:
* SERVICE-D from scope scope-child
  * SERVICE-C1 from scope scope-child
    * SERVICE-B from scope [root]
      * SERVICE-A1 from scope [root]
      * SERVICE-A2 from scope [root]
  * SERVICE-C2 from scope scope-child
    * SERVICE-B from scope [root]
      * SERVICE-A1 from scope [root]
      * SERVICE-A2 from scope [root]

Dependents:
* SERVICE-F from scope scope-child
  * SERVICE-G from scope scope-child

Here is the output of do.ExplainInjector(Injector) ๐Ÿ‘‡

Scope ID: scope-id-root
Scope name: [root]

DAG:

\_ [root] (ID: scope-id-root)
    * ๐Ÿ˜ด SERVICE-A1
    * ๐Ÿ˜ด SERVICE-A2
    * ๐Ÿ˜ด SERVICE-B
    |
    |
     \_ scope-0 (ID: scope-id-0)
         |
         |
         |\_ scope-1a (ID: scope-id-1a)
         |    * ๐Ÿ˜ด SERVICE-C1
         |    * ๐Ÿ˜ด SERVICE-C2
         |    * ๐Ÿ˜ด SERVICE-D
         |    * ๐Ÿ˜ด SERVICE-E
         |    * ๐Ÿ” SERVICE-EAGER-VALUE
         |    |
         |    |
         |    |\_ scope-2a (ID: scope-id-2a)
         |    |    * ๐Ÿ˜ด SERVICE-LAZY-HEALTH ๐Ÿฅ
         |    |    * ๐Ÿญ SERVICE-TRANSIANT-SIMPLE
         |    |     
         |    |
         |     \_ scope-2b (ID: scope-id-2b)
         |         * ๐Ÿ˜ด SERVICE-LAZY-SHUTDOWN ๐Ÿ™…
         |          
         |
          \_ scope-1b (ID: scope-id-1b)
              * ๐Ÿ˜ด SERVICE-F

The emoji prefix indicates the service is either Lazy, Eager, or Transiant. The EOL emoji indicates the service implements do.HealthChecker and/or do.Shutdowner.

codecov-commenter commented 9 months ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 90.15352% with 186 lines in your changes missing coverage. Please review.

Project coverage is 87.56%. Comparing base (9831445) to head (ec49e08). Report is 15 commits behind head on master.

Files Patch % Lines
service_alias.go 63.93% 40 Missing and 4 partials :warning:
virtual_scope.go 40.47% 25 Missing :warning:
scope.go 93.44% 17 Missing and 7 partials :warning:
service_eager.go 75.67% 14 Missing and 4 partials :warning:
root_scope.go 87.40% 17 Missing :warning:
invoke.go 94.00% 8 Missing and 4 partials :warning:
di.go 87.17% 10 Missing :warning:
stacktrace/stacktrace.go 86.88% 5 Missing and 3 partials :warning:
service_lazy.go 93.20% 5 Missing and 2 partials :warning:
di_explain.go 97.73% 5 Missing and 1 partial :warning:
... and 5 more

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #45 +/- ## ========================================== - Coverage 88.83% 87.56% -1.27% ========================================== Files 6 23 +17 Lines 430 2043 +1613 ========================================== + Hits 382 1789 +1407 - Misses 40 225 +185 - Partials 8 29 +21 ``` | [Flag](https://app.codecov.io/gh/samber/do/pull/45/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Samuel+Berthe) | Coverage ฮ” | | |---|---|---| | [unittests](https://app.codecov.io/gh/samber/do/pull/45/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Samuel+Berthe) | `87.56% <90.15%> (-1.27%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Samuel+Berthe#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

GreyXor commented 8 months ago

Hello, that's a really great news! Thanks a lot. I can't use the branch directly atm

go get github.com/samber/do@v2-๐Ÿš€
go: github.com/samber/do@v2-๐Ÿš€: invalid version: version "v2-๐Ÿš€" invalid: disallowed version string

workaround: go get github.com/samber/do@d722cd623c629ebfaf5b2032f998d3ae1b176009

danilobuerger commented 8 months ago

It would be nice to provide context o healthcheck and shutdown.

samber commented 8 months ago

@danilobuerger can you elaborate on your use case?

danilobuerger commented 8 months ago

@danilobuerger can you elaborate on your use case?

Sure, I want to provide a context to be able to use cancellation.

GreyXor commented 8 months ago

My last workaround don't work anymore. I've followed this guide https://github.com/samber/do/blob/v2-%F0%9F%9A%80/MIGRATION-V1-TO-V2.md and also the v2 readme : https://github.com/samber/do/blob/v2-%F0%9F%9A%80/README.md But I get this error go: github.com/samber/do@v2: no matching versions for query "v2" when I run go get github.com/samber/do@v2

samber commented 8 months ago

I renamed the package samber/do to samber/do/v2, so the latest command is: go get github.com/samber/do/v2@a158d2d9f84b4a1973db49e959e476aca996e04a

Adding an emoji in the branch name was definitely a bad idea ๐Ÿ˜†

GreyXor commented 8 months ago

I renamed the package samber/do to samber/do/v2, so the latest command is: go get github.com/samber/do/v2@a158d2d9f84b4a1973db49e959e476aca996e04a

Adding an emoji in the branch name was definitely a bad idea ๐Ÿ˜†

Not your fault, go get should be UTF-8 ready imho.

GreyXor commented 8 months ago

@samber, Since I've designed the ShutdownOnSignals* function to accommodate any signals, we've maintained the *OnSIGTERM for compatibility purposes. However, now that we have the option to revise the API (by breaking it), I believe all the shutdownOnSIGTERMs are unnecessary.

I therefore propose deleting ShutdownOnSIGTERM, ShutdownOnSIGTERMWithContext, ShutdownOnSIGTERMOrInterrupt and ShutdownOnSIGTERMOrInterruptWithContext.

What do you think about ? I feel that they are redundant

danilobuerger commented 7 months ago

One thing I noticed is that the pattern of Accept interfaces, return structs doesnt seem possible:

type EngineService interface{}

func NewEngineService(i *do.Injector) (*engineServiceImplem, error) {
    return &engineServiceImplem{}, nil
}

type engineServiceImplem struct {}

Later this fails:

do.Provide[EngineService](injector, NewEngineService)
samber commented 7 months ago

@danilobuerger v2 will make it possible to respect this pattern:

do.As[*engineServiceImplem, EngineService](injector)
// or
do.InvokeAs[EngineService](injector)

See the function comments for the full explanation.

matdurand commented 7 months ago

Hey, great work on the lib!

Any chance you have plans to support a concept similar to groups in Dig? (aka, registering a bunch of things of the same type, and asking for the list later). Pretty useful to do something like register a bunch of Routes associated with controllers in a DI container and then asking for all the routes to register them. That is super useful to decouple route creation, controller creation and servers for example.

https://uber-go.github.io/fx/value-groups/

samber commented 7 months ago

Hi @matdurand,

๐Ÿง I'm not familiar with the Value Groups. I need to "dig" (๐Ÿ˜) into it.

What is the value compared to do.ProvideValue(injector, []gin.IRoute{route1, route2, route3})? Do you sometimes use a value in a standalone fashion or always in a group?

do.ProvideValue is an eager loading, would you prefer injecting a provider, such as do.ProvideGroupItem(injector, NewRoute1)?

Injecting a type many times in different places, and then invoking them altogether looks interesting.

If you have any suggestions about a simple API for do, I would be happy to read you. ;)

Edit: Moving discussions to #52

GreyXor commented 6 months ago

Hello @samber, I'd like to directly pass the 'ch' channel to the ShutdownOnSignals function from an external source. This would allow for reuse the channel and enable additional operations elsewhere. I hope this modification is acceptable to you. I've submitted a pull request that enables us to specify our own channel. If the channel is nil, the function will create one for us.

https://github.com/samber/do/pull/56

danilobuerger commented 6 months ago

Hi @samber, really nice work! I tried it out today with one of our projects and just some feedback:

samber commented 5 months ago

I just made an update to make shutdown async and parallel.

Sub-scopes are shut down recursively first, then scope services. The service shutdown order is the opposite of the invocation order.

-> v2.0.0-beta.4

samber commented 5 months ago

It was weird getting all services back on Shutdown instead of only the ones with errors.

It has been fixed in the last commit. Now, it returns *do.ShutdownErrors which implements error. nil is returned if no errors were triggered.

I don't really understand the usecase for having Shutdown and HealthCheck without context. Anyone not using context could just pass in context.Background(). I guess the api would be a bit cleaner without the non-context funcs. I would argue the same for Healthchecker and Shutdowner. People could just ignore the passed in context. Also Shutdowner without error. People could just return nil

In term of developer experience I prefer exposing both alternative: Xxxx() and XxxxWithContext().

Lets vote:

(feel free to argue in comment)

Also, note that we would introduce a breaking change here. I suggest anybody to add interface static check for forward compability: var _ do.Shutdowner = (*MyService)(nil). Breaking changes would be much easier to track in your projects.

danilobuerger commented 5 months ago

Thanks for the amazing work! I will check out the new beta and report back.

samber commented 5 months ago

I use v2.0.0-beta.5 successfully at Screeb, in a big monolith of 500k-1m LoC with almost 200 services and a few scopes.

samber commented 5 months ago

What do you think about a shortcut for assembling providers in a package ?

// controllers/package.go

package controllers

var Pkg = do.Package(
    do.Lazy(NewService1),
    do.LazyNamed("foobar", NewService2),
)
// workers/package.go

package workers

var Pkg = do.Package(
    do.Lazy(NewService3),
    do.Eager(MyConfigValue),
        do.Transient(NewService5),
        do.As[*MyService, MyInterface],
)
// cmd/main.go

package main

import (
    "project/common"
    "project/workers"
    "project/controllers"
)

func main() {
    injector := do.New(
        common.Pkg,
    do.Lazy(NewService4),
    )
    Provide(injector, NewService6)

    scope1 := injector.Scope("workers", workers.Pkg)
    Provide(scope1, NewService7)

    scope2 := injector.Scope("controllers", controllers.Pkg)
}

I wonder if it's too much confusion. ๐Ÿซฃ

# prototypes
do.New(...func(do.Injector))
do.NewWithOpts(do.InjectorOpts, ...func(do.Injector))

injector.Scope(name string, ...func(do.Injector))
GreyXor commented 5 months ago

Hello @samber , I have an issue since the new parallel shutdown feature.

I have two services, Config and Event. Where Event is a dependency of Config.

type ConfigService struct {
    Event *event.Service
}

# In my Config's shutdown method, I calling anEvent's method.

With v2.0.0-beta.3, it's working as expected because the actual reverse order is respected. With v2.0.0-beta.5, it's randomly not working because Event is randomly Shutdown before Config. (because of the async)

How to disable the parallel shutdown behavior ?

mbark commented 4 months ago

Thank you so much for a great package @samber . Both this and lo I think have really nice interfaces and documentation, big kudos.

I've started integrating this into our code but has run into a problem. The problem appears to be that you can't override a value that will be used in InvokeAs.

We have CoreService that depends on NumberService (an interface). 

func New(injector do.Injector) (*CoreService, error) {
    return &CoreService{
        Number:           do.MustInvokeAs[poolv2.NumberService](injector),
    }, nil
}

We provide a default NumberService implementation.

do.Provide(injector, func(injector do.Injector) (*numberservice.NumberService, error)
    return &numberservice.NumberService{}, nil
})

However, for our unit tests we want to swap out the NumberService in favor of one generated by gomock.

numbersvc := mock_poolv2.NewMockNumberService(ctrl)

This mocked numbersvc I then want to be the one that is used when we do the MustInvokeAs. If I had used MustInvoke and explicitly register the struct and mock for the interface type it works.

// explicitly provide the service as the interface
do.ProvideValue[poolv2.NumberService](injector, numbersvc)

func New(injector do.Injector) (*CoreService, error) {
    return &CoreService{
        // explicitly ask for the interface type
        Number:           do.MustInvoke[poolv2.NumberService](injector),
    }, nil
}

However, that doesn't really fulfill the idea of "Accept interfaces, return structs" and also means our test code forces us to adapt our production code.

Is there some other way around this issue that I'm missing? Ideally I would be able to use something like OverrideAs to override a specific interface that is requested.

d-enk commented 2 months ago

@mbark you just can override poolv2.NumberService by name

do.OverrideNamedValue(i, do.NameOf[poolv2.NumberService](), mock_poolv2.NewMockNumberService(ctrl))
d-enk commented 2 months ago

Don't you think that in InvokeAs is returning a random a bad idea?

it's just dangerous.

What about returning an error if not only 1 is found?

samber commented 2 months ago

We need to push this PR forward.

I created a list of issues that must be addressed before v2 release:

Design:

Bugs:

Tasks/Features:

Doc:

GreyXor commented 2 months ago

Hello @samber,

Healthcheck not working properly when calling do.DefaultRootScope.HealthCheck() from Echo router API. I have this /healthcheck endpoint that should return the healthcheck errors list. I couldn't make an example code with the problem. I just lose the Step-by-step debugger thread here: https://github.com/samber/do/blob/v2-%F0%9F%9A%80/scope.go#L189.

Seems to work as expected when it's called from my main thread and not from my API (in my main.go for exemple)

but this, not working

func (a API) GetHealthcheck(_ context.Context, _ openapi.GetHealthcheckRequestObject) (openapi.GetHealthcheckResponseObject, error) {
    err := do.DefaultRootScope.HealthCheck()
    // never reached
    return openapi.GetHealthcheck200JSONResponse{}, nil
}