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

fx.Evaluate: Support dynamic graphs by returning fx.Option from constructors #1132

Open
abhinav opened this issue Nov 3, 2023 · 3 comments · May be fixed by #1259
Open

fx.Evaluate: Support dynamic graphs by returning fx.Option from constructors #1132

abhinav opened this issue Nov 3, 2023 · 3 comments · May be fixed by #1259
Assignees

Comments

@abhinav
Copy link
Collaborator

abhinav commented Nov 3, 2023

Feature request

Is your feature request related to a problem? Please describe.

This scenario was previously described in #1131. It's reproduced below to save a click.

Suppose a program has a Store interface with two implementations: a memory-backed version and a Redis-backed version.

type Store interface {
	Get(k string) (v string, error)
	Set(k, v string) error
}

type MemoryStore struct{ m map[string]string }

func NewMemoryStore() *MemoryStore

type RedisStore struct{ /* ... */ }

func NewRedisStore(*redis.Client) *RedisStore

The memory store has no dependencies, while the Redis store depends on a Redis client.

flowchart LR
  MemoryStore
  RedisStore -->|NewRedisStore| RedisClient[*redis.Client]
Loading

The application wants to pick between the two implementations based on a configuration parameter.
Imagine:

func NewStore(cfg Config, memStore *MemoryStore, rediStore *RedisStore) Store {
	if cfg.Development {
		return memStore
	}
	return redisStore
}

However, as a result of this, NewStore has a hard transitive dependency on the Redis client, even if the Redis store won't be used.

flowchart LR
  MemoryStore
  RedisStore -->|NewRedisStore| RedisClient[*redis.Client]
  Store -->|NewStore| Config & MemoryStore & RedisStore
  Store -.-> RedisClient
Loading

Ideally, NewStore should be able to specify which dependency set it wants to use after looking at the configuration.

Describe the solution you'd like

I'd like to propose adding a new intrinsic operation to the Fx model to accompany the existing intrinsics: Provide, Invoke, and Decorate.

package fx

func Evaluate(...any) fx.Option

The new operation, tentatively named fx.Evaluate, will represent an unevaluated part of the graph.
image

Functions passed to fx.Evaluate will always be evaluated. They will feed new information to the graph in the form of fx.Option return values.
These new operations may form connections that previously did not exist.
image

With this operation, the scenario described above would be solved like so:

func NewStore(cfg Config) fx.Option {
    if cfg.Development {
        return fx.Provide(func(s *MemoryStore) Store { return s })
    }
    return fx.Provide(func(s *RedisStore) Store { return s })
}

// Or alternatively:

func NewStore(cfg Config) fx.Option {
    if cfg.Development {
        return fx.Provide(
            fx.Annotate(NewMemoryStore, fx.As(new(Store))),
        )
    }
    return fx.Provide(
        fx.Annotate(NewRedisStore, fx.As(new(Store))),
    )
}

// In main:

fx.New(
    // ...
    fx.Evaluate(NewStore),
)

Describe alternatives you've considered

fx.Replace was considered but is insufficient for this functionality.
It cannot conditionally severe the dependency between NewStore and *redis.Client.
See this comment.

Is this a breaking change?
No, this is not a breaking change.

Additional context
This functionality has been brought up before and has been referred to with names such as: monadic graph, dynamic provides, late provides.

There was a prior attempt to implement this in #699 (tracked in #698). This attempt was rejected because of the following reasons per this comment.

  • There was risk of UX confusion by overloading fx.Provide. This is not an issue if we introduce a new intrinsic.
  • The internals of the library were actively being reworked for fx.Decorate. This is done now.
  • There were concerns about how "late provides" would interact with lifecycles. Again, this is not an issue with a new intrinsic because new expectations can be established: Evaluate always runs, so lifecycle events there always run.

Fx startup roughly takes the following shape:

provides, decorates, invokes = [...], [...], [...]

provideAll(provides)
decorateAll(decorates)
invokeAll(invokes)

start()

With the new intrinsic, this could effectively become:

provides, decorates, invokes, evaluates = [...], [...], [...], [...]

provideAll(provides)
decorateAll(decorates)

while len(evaluates) > 0:
  e = pop(evaluates)
  ps, ds, is, es = evaluate(e)
  provideAll(ps)
  decorateAll(ds)
  invokes.append(is...)
  evaluates.append(es...)

invokeAll(invokes)
@sywhang sywhang self-assigned this Nov 21, 2023
@Southclaws
Copy link

I've run across the exact same scenario, with caches but also things like SMS, Email, S3 mocks. Fx seems to be fighting against idiomatic interface usage here and makes it difficult to effectively use interfaces to describe varying implementations.

The workaround I've used for now is to simply construct everything together, rather than provide a redis client as a separate dependency in the graph. This does work fine for now and I can't see it becoming a bigger problem unless this issue arises with deeper dependency trees (though I tend to avoid that with this sort of infrastructure)

func Build() fx.Option {
	return fx.Options(
		fx.Provide(func(cfg config.Config) (Store, error) {
			switch cfg.CacheProvider {
			case "":
				return local.New(), nil

			case "redis":
				client, err := rueidis.NewClient(rueidis.ClientOption{
					InitAddress: []string{cfg.RedisHost},
				})
				if err != nil {
					return nil, fault.Wrap(err, fmsg.With("failed to connect to redis"))
				}

				return redis.New(client), nil
			}

			panic("unknown cache provider: " + cfg.CacheProvider)
		}),
	)
}

@jbcpollak
Copy link

it seems to me it would be more intuitive to invert this by determining the provider first, then passing that into fx:

func getCacheProvider(cfg config.Config) func() CacheProvider {
	switch cfg.CacheProvider {
	case "":
		return NewLocalCacheProvider // a function

	case "redis":
		return NewRedisCacheProvider // another factory
	}
}

fx.Provide(getCacheProvider(cfg))

I think you could take this a step further by deferring getCacheProvider execution and having fx handle passing in the cfg.

@Southclaws
Copy link

The problem is the cfg param is required to determine which provider to run, but it's also a dependency so either I move configs out of fx entirely (huge amount of monotonous work, not worth it) or hack around it.

@abhinav abhinav linked a pull request Jan 9, 2025 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants