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

Adds OnStart/OnStop lifecycle Annotations #895

Merged
merged 16 commits into from Jul 27, 2022

Conversation

jasonmills
Copy link
Contributor

@jasonmills jasonmills commented Jul 1, 2022

This provides two new annotations, OnStart and OnStop. Each will append their respective hooks into the application Lifecycle when the provided option function is executed.

Usage:

type Interface interface {
    Start(context.Context) error
    Stop(context.Context) error
}

func New() (Interface, error)

var Module = fx.Provide(fx.Annotate(
    New,
    fx.OnStart(func(ctx context.Context, server Interface) error {
      return server.Start(ctx)
    }),
    fx.OnStop(func(ctx context.Context, server Interface) error {
       return server.Stop(ctx)
   }),
))   

Which is functionally equivalent to:

var Module = fx.Provide(
  func(lifecycle fx.Lifecycle) (Interface, error) {
    server, err := New()
    lifecycle.Append(fx.Hook{
      OnStart: func(ctx context.Context) error { return server.Start(ctx) }, 
      OnStop: func(ctx context.Context) error { return server.Stop(ctx) },
    }
    return server, err
  },
)  

Note for reviewers:

  • The underlying implementation is unified for each hook type and may be easily extended should additional hook fields be added to the Hook type.
  • This slightly modifies the parameter implementation of the private annotation type, this was required in order to make the hook annotation types able to detect when a given annotated function provides a required hook (closure) parameter. Without this any hook that takes a dependency on a type that is constructed in the same annotation would trigger a circular dependency.

@CLAassistant
Copy link

CLAassistant commented Jul 1, 2022

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jul 1, 2022

Codecov Report

Merging #895 (67de07f) into master (4f51a19) will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #895      +/-   ##
==========================================
+ Coverage   98.53%   98.71%   +0.18%     
==========================================
  Files          30       30              
  Lines        1163     1328     +165     
==========================================
+ Hits         1146     1311     +165     
  Misses         11       11              
  Partials        6        6              
Impacted Files Coverage Δ
annotated.go 99.69% <100.00%> (+0.30%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

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

Good start. Left some comments.

Also, there a couple of missing test cases that would be nice to cover here:

  • Providing nested hooks and validating the execution order
  • fx.Invoke can also take in an fx.Annotated function. I don't think you'll need much (if any) modifications to the existing code, but can you add a test case for that?

annotated_test.go Outdated Show resolved Hide resolved
annotated_test.go Outdated Show resolved Hide resolved
annotated_test.go Outdated Show resolved Hide resolved
annotated_test.go Outdated Show resolved Hide resolved
annotated_test.go Outdated Show resolved Hide resolved
annotated.go Outdated Show resolved Hide resolved
annotated_test.go Outdated Show resolved Hide resolved
annotated.go Outdated Show resolved Hide resolved
annotated.go Outdated Show resolved Hide resolved
annotated.go Outdated Show resolved Hide resolved
@sywhang
Copy link
Contributor

sywhang commented Jul 1, 2022

also it looks like this branch is out-of-date with the master branch - can you rebase on master?

annotated_test.go Outdated Show resolved Hide resolved
annotated_test.go Outdated Show resolved Hide resolved
annotated.go Show resolved Hide resolved
annotated.go Outdated Show resolved Hide resolved
annotated.go Outdated Show resolved Hide resolved
Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

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

👏 Coming together nicely - left one more round of reviews, mainly around cleanliness and readability.

Also try to edit the PR message here to include a brief description of the API and how one might go about using this, since a lot of people come across the PRs as part of the documentation (which we do lack but that's a separate story).

annotated.go Outdated Show resolved Hide resolved
annotated.go Outdated Show resolved Hide resolved
annotated.go Outdated Show resolved Hide resolved
annotated.go Outdated Show resolved Hide resolved
annotated_test.go Outdated Show resolved Hide resolved
annotated_test.go Outdated Show resolved Hide resolved
annotated_test.go Outdated Show resolved Hide resolved
annotated.go Show resolved Hide resolved
annotated.go Outdated Show resolved Hide resolved
annotated.go Outdated Show resolved Hide resolved
@jasonmills jasonmills merged commit f002e36 into uber-go:master Jul 27, 2022
abhinav pushed a commit that referenced this pull request Jul 27, 2022
This provides two new annotations, OnStart and OnStop.
Each will append their respective hooks into the application Lifecycle
when the provided option function is executed.

Usage:

```go
type Interface interface {
    Start(context.Context) error
    Stop(context.Context) error
}

func New() (Interface, error)

var Module = fx.Provide(fx.Annotate(
    New,
    fx.OnStart(func(ctx context.Context, server Interface) error {
      return server.Start(ctx)
    }),
    fx.OnStop(func(ctx context.Context, server Interface) error {
       return server.Stop(ctx)
   }),
))
```

Which is functionally equivalent to:

```go
var Module = fx.Provide(
  func(lifecycle fx.Lifecycle) (Interface, error) {
    server, err := New()
    lifecycle.Append(fx.Hook{
      OnStart: func(ctx context.Context) error { return server.Start(ctx) },
      OnStop: func(ctx context.Context) error { return server.Stop(ctx) },
    }
    return server, err
  },
)
```

Note for reviewers:

* The underlying implementation is unified for each hook type and may be
  easily extended should additional hook fields be added to the Hook
  type.
* This slightly modifies the parameter implementation of the private
  annotation type, this was required in order to make the hook
  annotation types able to detect when a given annotated function
  provides a required hook (closure) parameter. Without this any hook
  that takes a dependency on a type that is constructed in the same
  annotation would trigger a circular dependency.

Refs GO-201
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants