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

Make sure that constructor only creates one Dependency (and an optional error) #729

Open
talgendler opened this issue Feb 28, 2021 · 3 comments
Labels
v2 Issues/problems to address if we ever start looking at a v2

Comments

@talgendler
Copy link

The ability to create several dependencies from a single constructor function is very useful, but it makes it very hard to replace a dependency during tests.

func CreateDep() (dep.One, dep.Two, error) {...}

Currently we maintain several dependency trees, one for production/main and others for tests.
This is a manual process, where one needs to constantly remember to adjust trees when some dependencies change.
I read in previous issues that there were attempts to replace dependencies in tests, but without any real progress on that.
That's why I propose to limit the amount of dependencies one can produce from a single constructor. This will make it much easier to "ignore" a constructor function and "replace" it, impacting only 1 dependency.
I know this is a breaking change and that's why I propose to include this as part of the major release.

@abhinav
Copy link
Collaborator

abhinav commented Nov 4, 2021

Hi @talgendler, apologies for the delay.
We would strongly prefer not to make a breaking change to Fx at this time.
A majority of our internal Go ecosystem is built upon and relies on Fx,
and introducing breaking changes to Fx without a clear migration path is an expensive prospect.

Is this something that would be better accomplished with a linter?
I suspect with go/analysis,
you can write a linter that inspects types of arguments to fx.Provide(..)
and fails if they produce multiple non-error results.

@talgendler
Copy link
Author

talgendler commented Nov 4, 2021

I'm not suggesting to make a breaking change in the current version. But if there are plans to introduce a v2 it might be a candidate. The reason for this request is that it will make tests much easier to implement. You could create a "production" branch, but then in tests you could say to fx - please replace this interface A with this mock. We can have a code in fx (or only for tests) that will find a constructor providing this A dependency and instead of invoking it will provide the mock. It's only possible if this constructor provides only one dependency and an optional error. If this constructor provides several dependencies than we will need to make sure to replace all of them and that is much harder to ensure.

@abhinav
Copy link
Collaborator

abhinav commented Nov 4, 2021

Fair enough. I'm going to tag this with the v2 label I just created.

Separately, I do want to mention that we're trying to do some work in service of #653 (lack of which seems to be the reason this feature might be needed). I can't commit to a timeline yet, but if we ship that feature, this issue may become moot.

@abhinav abhinav added the v2 Issues/problems to address if we ever start looking at a v2 label Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2 Issues/problems to address if we ever start looking at a v2
Development

No branches or pull requests

2 participants