Skip to content

Commit

Permalink
refac: DRY up New, reuse *All methods, delete modules slice (#1258)
Browse files Browse the repository at this point in the history
The pattern we appear to have for Fx operations (Provide, Invoke, etc.)
take the form:

    func (*module) thing(t thing)

    func (m *module) thingAll() {
        // perform module-local version of operation
        for _, t := range m.things {
            m.thing(t)
        }

        // recurse into children
        for _, m := range m.modules {
            m.${operation}All()
        }
    }

This means that the following two are equivalent:

    // 1
    for _, m := range app.modules {
        m.thingAll()
    }

    // 2
    app.root.thingAll()

Except (2) is DRYer.

This cleans up New by relying on root-level `*All` methods
instead of manually iterating over modules.

Making this change also highlighted that 'app.modules'
only ever has one entry: the root module.
So we can delete that field from App as well.

Finally, this also renames:

    constructAllCustomLoggers -> installAllEventLoggers
    constructCustomLogger     -> installEventLogger
    executeInvokes            -> invokeAll
    executeInvoke             -> invoke
  • Loading branch information
abhinav authored Jan 9, 2025
1 parent 928af08 commit 0ad8a04
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 25 deletions.
22 changes: 6 additions & 16 deletions app.go
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,6 @@ type App struct {

container *dig.Container
root *module
modules []*module

// Timeouts used
startTimeout time.Duration
Expand Down Expand Up @@ -446,7 +445,6 @@ func New(opts ...Option) *App {
log: logger,
trace: []string{fxreflect.CallerStack(1, 2)[0].String()},
}
app.modules = append(app.modules, app.root)

for _, opt := range opts {
opt.apply(app.root)
Expand Down Expand Up @@ -475,10 +473,7 @@ func New(opts ...Option) *App {
}

app.container = dig.New(containerOptions...)

for _, m := range app.modules {
m.build(app, app.container)
}
app.root.build(app, app.container)

// Provide Fx types first to increase the chance a custom logger
// can be successfully built in the face of unrelated DI failure.
Expand All @@ -490,31 +485,26 @@ func New(opts ...Option) *App {
})
app.root.provide(provide{Target: app.shutdowner, Stack: frames})
app.root.provide(provide{Target: app.dotGraph, Stack: frames})
app.root.provideAll()

for _, m := range app.modules {
m.provideAll()
}

// Run decorators before executing any Invokes -- including the one
// inside constructCustomLogger.
// Run decorators before executing any Invokes
// (including the ones inside installAllEventLoggers).
app.err = multierr.Append(app.err, app.root.decorateAll())

// If you are thinking about returning here after provides: do not (just yet)!
// If a custom logger was being used, we're still buffering messages.
// We'll want to flush them to the logger.

// custom app logger will be initialized by the root module.
for _, m := range app.modules {
m.constructAllCustomLoggers()
}
app.root.installAllEventLoggers()

// This error might have come from the provide loop above. We've
// already flushed to the custom logger, so we can return.
if app.err != nil {
return app
}

if err := app.root.executeInvokes(); err != nil {
if err := app.root.invokeAll(); err != nil {
app.err = err

if dig.CanVisualizeError(err) {
Expand Down
17 changes: 8 additions & 9 deletions module.go
Original file line number Diff line number Diff line change
Expand Up @@ -253,11 +253,11 @@ func (m *module) supply(p provide) {
}

// Constructs custom loggers for all modules in the tree
func (m *module) constructAllCustomLoggers() {
func (m *module) installAllEventLoggers() {
if m.logConstructor != nil {
if buffer, ok := m.log.(*logBuffer); ok {
// default to parent's logger if custom logger constructor fails
if err := m.constructCustomLogger(buffer); err != nil {
if err := m.installEventLogger(buffer); err != nil {
m.app.err = multierr.Append(m.app.err, err)
m.log = m.fallbackLogger
buffer.Connect(m.log)
Expand All @@ -269,12 +269,11 @@ func (m *module) constructAllCustomLoggers() {
}

for _, mod := range m.modules {
mod.constructAllCustomLoggers()
mod.installAllEventLoggers()
}
}

// Mirroring the behavior of app.constructCustomLogger
func (m *module) constructCustomLogger(buffer *logBuffer) (err error) {
func (m *module) installEventLogger(buffer *logBuffer) (err error) {
p := m.logConstructor
fname := fxreflect.FuncName(p.Target)
defer func() {
Expand All @@ -297,23 +296,23 @@ func (m *module) constructCustomLogger(buffer *logBuffer) (err error) {
})
}

func (m *module) executeInvokes() error {
func (m *module) invokeAll() error {
for _, m := range m.modules {
if err := m.executeInvokes(); err != nil {
if err := m.invokeAll(); err != nil {
return err
}
}

for _, invoke := range m.invokes {
if err := m.executeInvoke(invoke); err != nil {
if err := m.invoke(invoke); err != nil {
return err
}
}

return nil
}

func (m *module) executeInvoke(i invoke) (err error) {
func (m *module) invoke(i invoke) (err error) {
fnName := fxreflect.FuncName(i.Target)
m.log.LogEvent(&fxevent.Invoking{
FunctionName: fnName,
Expand Down

0 comments on commit 0ad8a04

Please sign in to comment.