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

Provide Fx types before user types #1191

Merged
merged 2 commits into from
Apr 19, 2024

Conversation

JacobOaks
Copy link
Contributor

@JacobOaks JacobOaks commented Apr 18, 2024

Consider the following example:

func opts() fx.Option {
        return fx.Options(
                fx.WithLogger(func(fx.Lifecycle) fxevent.Logger {
                        return &fxevent.ConsoleLogger{ W: os.Stdout }
                }),
                fx.Provide(func() string { return "" }),
                fx.Provide(func() string { return "" }),
        )
}

func main() {
        fx.New(opts()).Run()
}

The relevant issue to surface to the user is that they are double providing the same type. However, the actual error message is:

[Fx] ERROR              Failed to start: the following errors occurred:
 -  fx.Provide(main.opts.func3()) from:
    main.opts
        /home/user/go/src/scratch/fx_provide_order/main.go:17
    main.main
        /home/user/go/src/scratch/fx_provide_order/main.go:22
    runtime.main
        /opt/go/root/src/runtime/proc.go:271
    Failed: cannot provide function "main".opts.func3
(/home/user/go/src/scratch/fx_provide_order/main.go:17): cannot provide
string from [0]: already provided by "main".opts.func2
(/home/user/go/src/scratch/fx_provide_order/main.go:16)
 -  could not build arguments for function
"go.uber.org/fx".(*module).constructCustomLogger.func2
        /home/user/go-repos/pkg/mod/go.uber.org/[email protected]/module.go:292:
    failed to build fxevent.Logger:
    missing dependencies for function "main".opts.func1
        /home/user/go/src/scratch/fx_provide_order/main.go:11:
    missing type:
        - fx.Lifecycle (did you mean to Provide it?)

Which contains an additional error related to how the custom logger could not be built.

This is because Fx will try to continue to build the custom logger in the face of DI failure, theoretically to report issues through the right channels. But after an error occurs when providing anything, Fx refuses to provide any more types - leading to a subsequent error when trying to build this custom logger that depends on the fx.Lifecycle type.

This is a common issue that can be misleading for new engineers debugging their fx apps.

I couldn't find any particular reason why user-provided provides are registered before these Fx types, so this PR switches this ordering so that custom loggers can still be built if they rely on the Fx types, which cleans up the error message:

[Fx] ERROR              Failed to start: fx.Provide(main.opts.func3())
from:
main.opts
        /home/user/go/src/scratch/fx_provide_order/main.go:17
main.main
        /home/user/go/src/scratch/fx_provide_order/main.go:22
runtime.main
        /opt/go/root/src/runtime/proc.go:271
Failed: cannot provide function "main".opts.func3
(/home/user/go/src/scratch/fx_provide_order/main.go:17): cannot provide
string from [0]: already provided by "main".opts.func2
(/home/user/go/src/scratch/fx_provide_order/main.go:16)

@JacobOaks JacobOaks changed the title Provide internal types first Provide Fx types before user types Apr 18, 2024
@JacobOaks JacobOaks marked this pull request as ready for review April 18, 2024 21:53
Copy link

codecov bot commented Apr 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.73%. Comparing base (9814dd3) to head (4626474).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1191   +/-   ##
=======================================
  Coverage   98.73%   98.73%           
=======================================
  Files          31       31           
  Lines        2851     2851           
=======================================
  Hits         2815     2815           
  Misses         29       29           
  Partials        7        7           

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

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.

Did we validate this in internal CI? I don't see a reason it should affect anything but in case.

app.go Show resolved Hide resolved
app_test.go Outdated Show resolved Hide resolved
app_test.go Show resolved Hide resolved
Consider the following example:
```go
func opts() fx.Option {
        return fx.Options(
                fx.WithLogger(func(fx.Lifecycle) fxevent.Logger {
                        return &fxevent.ConsoleLogger{ W: os.Stdout }
                }),
                fx.Provide(func() string { return "" }),
                fx.Provide(func() string { return "" }),
        )
}

func main() {
        fx.New(opts()).Run()
}
```

The relevant issue to surface to the user is that they are double providing
the same type. However, the actual error message is:
```
[Fx] ERROR              Failed to start: the following errors occurred:
 -  fx.Provide(main.opts.func3()) from:
    main.opts
        /home/user/go/src/scratch/fx_provide_order/main.go:17
    main.main
        /home/user/go/src/scratch/fx_provide_order/main.go:22
    runtime.main
        /opt/go/root/src/runtime/proc.go:271
    Failed: cannot provide function "main".opts.func3
(/home/user/go/src/scratch/fx_provide_order/main.go:17): cannot provide
string from [0]: already provided by "main".opts.func2
(/home/user/go/src/scratch/fx_provide_order/main.go:16)
 -  could not build arguments for function
"go.uber.org/fx".(*module).constructCustomLogger.func2
        /home/user/go-repos/pkg/mod/go.uber.org/[email protected]/module.go:292:
    failed to build fxevent.Logger:
    missing dependencies for function "main".opts.func1
        /home/user/go/src/scratch/fx_provide_order/main.go:11:
    missing type:
        - fx.Lifecycle (did you mean to Provide it?)
```
Which contains an additional error related to how the custom logger
could not be built.

This is because Fx will try to continue to build the custom logger
in the face of DI failure, theoretically to report issues
through the right channels. But after an error occurs when
providing anything, Fx refuses to provide any more types - leading to
a subsequent error when trying to build this custom logger
that depends on the `fx.Lifecycle` type.

This is a common issue that can be misleading for new engineers
debugging their fx apps.

I couldn't find any particular reason why user-provided provides
are registered before these Fx types, se this PR switches this ordering
so that custom loggers can still be built if they rely on the Fx types,
which cleans up the error message:
```
[Fx] ERROR              Failed to start: fx.Provide(main.opts.func3())
from:
main.opts
        /home/user/go/src/scratch/fx_provide_order/main.go:17
main.main
        /home/user/go/src/scratch/fx_provide_order/main.go:22
runtime.main
        /opt/go/root/src/runtime/proc.go:271
Failed: cannot provide function "main".opts.func3
(/home/user/go/src/scratch/fx_provide_order/main.go:17): cannot provide
string from [0]: already provided by "main".opts.func2
(/home/user/go/src/scratch/fx_provide_order/main.go:16)
```
@JacobOaks
Copy link
Contributor Author

JacobOaks commented Apr 19, 2024

Did we validate this in internal CI? I don't see a reason it should affect anything but in case.

Yes, this has been validated in internal CI @sywhang

@JacobOaks JacobOaks merged commit 95cbe83 into uber-go:master Apr 19, 2024
12 checks passed
@sywhang
Copy link
Contributor

sywhang commented Apr 23, 2024

@JacobOaks do you want to cut a release?

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.

2 participants