Red Team Feedback

[HelloGrayson]

Did a Red Team exercise with @peter-edge on the latest RC - the overall experience was super positive 🎉 🎈 🍾 , and I was able to capture lots of good feedback, mostly relating to hardening the API before release.

All of these items should be considered "take it or leave it" - obviously the release is closing in fast, so if we reject all of these or just take on a few, that is totally fine 👍 I've lined up PRs so that if we do want a few, we can take them on quickly.

Directions

1. Read this PR description first
2. VOTE on each PR
3. Comment/Discuss at the PR level

I will create a follow-up meeting where we can make a decision across the board here, either rejecting and closing the PRs or accepting. This this issue will be closed when we cut v1.0.0 today.

Feedback

Here are my notes from the session:

Further Reduce API Surface

One of the things that was nice to see in the exercise was how someone new absorbed the API - we spent alot of time in the GoDoc looking at the public API. A big takeaway is that we should consider aggressively reducing the API surface, mainly providing less convenience so that the API would be even tinier.

For example, we found ourselves wondering how nice it would be to reduce the API surface to just fx.New, fx.Lifecycle/Hook, and the options for fx.New. Additional APIs can always be added later, and just like with go.uber.org/config, we should be asking what things we can survive without for a v1.0.0.

Consider deleting fx.Timeout #579 (accepted)

This helper is useful in testing, but it looks like we can ship without it. We should consider killing this func for that reason alone. The fx.Start and fx.Stop methods could even set the default timeout on a context.Background() if one wasn't attached already.

Consider deleting fx.App.Done #580 (rejected)

This is a convenience function we could survive without. Most customers will call fx.Run, which should be good enough for now. If signal handling becomes something many apps need to do manually, then we can consider adding this back. Until then - it represents a function we'll need to explain/teach, adds extra weight to godoc, and we won't be able to delete it.

Consider deleting fx.Extract #578 (rejected)

As far as we could tell, fx.Extract can be replaced with a single func passed to fx.Invoke, where the users treats that func as their original main. This lets them "extract" as many types as they need, use fx.Run instead of fx.Start/Stop, and gives the fx.In support by-way of fx.Invoke.

This is an interesting one - if we can somehow survive this way, then that's a big win because fx.Extract adds an alternate "invoke-like" pathway that we have to explain and teach.

Other Suggestions

Lot's of miscellaneous naming things along the way. Since we've been close to these APIs for awhile, I think we're sort of immune to seeing these. Nice to get an outsider reacting to them from the git-go.

Options passed to fx.New should use With* naming #581, #582, #583, #587 (rejected)

One of the things that was surprising was that options didn't begin with With*.

• fx.Provide -> fx.WithConstructors
• fx.Invoke -> fx.WithInvokes
• fx.Options -> fx.WithModule
• fx.Logger -> fx.WithLogger

I was pleasantly surprised how refreshing I found fx.WithConstructors, which is more verbose but much much easier to understand, and so will be better for end-users. I also found that the With* convention better matched the idea that these things aren't necessarily happening immediately, that instead you are instructing the fx.App to use these when it does it's thing.

Re:fx.Options: If the primary use for fx.Options is to define a module, then we should consider renaming it to fx.WithModule. In other words, it's easier to teach and recall concepts when we name things after how they are used, instead of what they are. I argued this point with the fx.Inject to fx.Extract rename.

fxtest should rename Must to Require #584 (accepted)

The Must* terminology implies that the func will panic, which is inaccurate here - we're calling into a testing.T to fail the test instead. Other testing libs seems to have adopted a Require* naming convention to indicate this - we should consider doing the same.

Run should return an error #585 (rejected)

It was surprising during the exercise that Run didn't return anything, instead panicking. While I view this as a nice convenience, the following might be more regular to users:

app := fx.New()
if err := fx.Run(); err != nil {
    log.Fatalf(err)
}

I could go either way on this one - since Fx is the very top-level thing, it doesn't personally bother me that we panic in fx.Run.

Move fx.Extract to it's own package #586 (rejected)

If we decide that we shouldn't delete fx.Extract, as mentioned in the previous item, then we should consider moving it to it's own package so that the top-level APIs contain only the "golden path" usage. Perhaps consider fxmigrate.Extract or something similar.

Also, we should rename Extract with WithExtract to match other options.

Rendered

I live-edited the GoDoc html in my browser to generate a preview of what the GoDoc/public API would look like after all this feedback:

[HelloGrayson]

We've gone through the proposals and marked them rejected/approved.

The approved PRs will make it into v1.0.0.

Thank you to the customers who provided this valuable feedback.

[HelloGrayson]

The accepted proposals have been merged!