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

[👷] Road to v2 🚀 #45

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

[👷] Road to v2 🚀 #45

wants to merge 108 commits into from

Conversation

samber
Copy link
Owner

@samber samber commented Oct 23, 2023

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. 😍

  • Reach 80% code coverage
  • Reach 85% code coverage
  • Reach 90% code coverage
  • See // @TODO in xxx_test.go files
  • More tests on nested scopes (clone, service invocation...)
  • More tests on virtual scopes

Coverage:

  • 2023-10-23: 83%
  • 2023-12-30: 87%
  • 2024-05-07: 89%

Breaking changes

  • Injector struct turned into an interface 😕
  • ListProvidedServices and ListInvokedServices now return 2 do.Edge arrays.
  • No more panic propagation on Provider error 😛
  • Service implicit names have been changed. BTW, you should never manipulate it! Eg: *mypkg.MyService -> *github.com/samber/example.MyService.
  • injector.ShutdownOnSignals and injector.ShutdownOnSIGTERM now return (os.Signal, error)

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.

Repository owner deleted a comment from codecov-commenter Oct 23, 2023
@samber
Copy link
Owner Author

samber commented Oct 23, 2023

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
Copy link

codecov-commenter commented Oct 23, 2023

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 with missing lines Patch % Lines
service_alias.go 63.93% 40 Missing and 4 partials ⚠️
virtual_scope.go 40.47% 25 Missing ⚠️
scope.go 93.44% 17 Missing and 7 partials ⚠️
service_eager.go 75.67% 14 Missing and 4 partials ⚠️
root_scope.go 87.40% 17 Missing ⚠️
invoke.go 94.00% 8 Missing and 4 partials ⚠️
di.go 87.17% 10 Missing ⚠️
stacktrace/stacktrace.go 86.88% 5 Missing and 3 partials ⚠️
service_lazy.go 93.20% 5 Missing and 2 partials ⚠️
di_explain.go 97.73% 5 Missing and 1 partial ⚠️
... and 5 more
Additional details and impacted files
@@            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 Coverage Δ
unittests 87.56% <90.15%> (-1.27%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GreyXor
Copy link
Contributor

GreyXor commented Oct 25, 2023

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
Copy link

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

@samber
Copy link
Owner Author

samber commented Oct 26, 2023

@danilobuerger can you elaborate on your use case?

@danilobuerger
Copy link

@danilobuerger can you elaborate on your use case?

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

@GreyXor
Copy link
Contributor

GreyXor commented Nov 1, 2023

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
Copy link
Owner Author

samber commented Nov 1, 2023

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
Copy link
Contributor

GreyXor commented Nov 2, 2023

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
Copy link
Contributor

GreyXor commented Nov 2, 2023

@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
Copy link

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
Copy link
Owner Author

samber commented Nov 28, 2023

@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.

@GreyXor
Copy link
Contributor

GreyXor commented May 10, 2024

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
}

@igorrius
Copy link

Hello @samber ,
there is a small issue with Logf - the first argument root is not a format parameter but already rendered.
image

@igorrius
Copy link

igorrius commented Dec 1, 2024

Hello, @samber !
Having a Zero return value for a no-error case would be nice there.
image

@GreyXor
Copy link
Contributor

GreyXor commented Dec 2, 2024

@samber Hello, thanks for all your work!
Initial ETA was end of January, which is is largely deadlined. Can you update it maybe ? No pressure at all, just curious about roadmap etc.

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.