factoryBean() has no way to declare singleton vs transient #417

Closed
seancorfield opened this Issue Jan 4, 2016 · 6 comments

Comments

Projects
None yet
1 participant
@seancorfield
Member

seancorfield commented Jan 4, 2016

DI/1 has historically assumed a factoryBean() is always a transient, but that has the nasty side-effect that you cannot do property injection with those beans (you can do constructor injection, which is a subtle difference -- see #408 for why).

There needs to be some way to tell DI/1 what the result of a factoryBean call should be.

@seancorfield seancorfield self-assigned this Jan 4, 2016

@seancorfield seancorfield added this to the 4.0 milestone Jan 4, 2016

@seancorfield

This comment has been minimized.

Show comment
Hide comment
@seancorfield

seancorfield Jan 4, 2016

Member

Another possibility is to relax the restriction on injecting transients, and to allow that for singletons (but still not for transients). That would solve the issue of injecting factory beans into singletons. But you might still want factory beans injected into transients? You could use constructor injection there to make that work.

Member

seancorfield commented Jan 4, 2016

Another possibility is to relax the restriction on injecting transients, and to allow that for singletons (but still not for transients). That would solve the issue of injecting factory beans into singletons. But you might still want factory beans injected into transients? You could use constructor injection there to make that work.

seancorfield added a commit that referenced this issue Jan 4, 2016

Relax transient injection rule #417
This addresses the common cases in a different way, but may not be
entirely desirable.
@seancorfield

This comment has been minimized.

Show comment
Hide comment
@seancorfield

seancorfield Jan 11, 2016

Member

Transients can now be property-injected into singletons. Transients still cannot be property-injected into transients. This solved the specific problem reported on Slack but doesn't really address the underlying issue of this ticket.

Naming is the hard part. factoryBean() is generic but we don't really want factoryTransient() and factorySingleton(), and added a new optional argument makes for ugly calls (because there are already optional arguments which can often be omitted).

Builder syntax would be a possibility if factoryBean() wasn't already set up for method chaining, although there's nothing to prevent a completely new set of methods being added to support a builder syntax...

Member

seancorfield commented Jan 11, 2016

Transients can now be property-injected into singletons. Transients still cannot be property-injected into transients. This solved the specific problem reported on Slack but doesn't really address the underlying issue of this ticket.

Naming is the hard part. factoryBean() is generic but we don't really want factoryTransient() and factorySingleton(), and added a new optional argument makes for ugly calls (because there are already optional arguments which can often be omitted).

Builder syntax would be a possibility if factoryBean() wasn't already set up for method chaining, although there's nothing to prevent a completely new set of methods being added to support a builder syntax...

@seancorfield

This comment has been minimized.

Show comment
Hide comment
@seancorfield

seancorfield Jan 12, 2016

Member

bf.declare("beanName").fromFactory(beanNameOrInstance,"method",optArgs).asSingleton() (or .asTransient() -- which would be the default).

Could also use this for other declarations: .aliasFor("originalName"), .asValue(someValue), instanceOf(dottedPath), .withOverrides(overrideStruct).

Member

seancorfield commented Jan 12, 2016

bf.declare("beanName").fromFactory(beanNameOrInstance,"method",optArgs).asSingleton() (or .asTransient() -- which would be the default).

Could also use this for other declarations: .aliasFor("originalName"), .asValue(someValue), instanceOf(dottedPath), .withOverrides(overrideStruct).

@seancorfield

This comment has been minimized.

Show comment
Hide comment
@seancorfield

seancorfield Jan 12, 2016

Member

.fromFactory() could also support a function/closure instead of bean & method I guess...

Member

seancorfield commented Jan 12, 2016

.fromFactory() could also support a function/closure instead of bean & method I guess...

@seancorfield

This comment has been minimized.

Show comment
Hide comment
@seancorfield

seancorfield Jan 12, 2016

Member

See #418 for that last piece.

Member

seancorfield commented Jan 12, 2016

See #418 for that last piece.

@seancorfield

This comment has been minimized.

Show comment
Hide comment
@seancorfield

seancorfield Jan 12, 2016

Member

Had to revert the transient injection relaxation: it broke the World Singles codebase in nasty, subtle ways -- and made me realize it could break other people's code in strange ways too!

Member

seancorfield commented Jan 12, 2016

Had to revert the transient injection relaxation: it broke the World Singles codebase in nasty, subtle ways -- and made me realize it could break other people's code in strange ways too!

seancorfield added a commit that referenced this issue Jan 12, 2016

Revert "Relax transient injection rule #417"
This can break reasonable, existing code. Specifically it breaks World
Singles code in very unpleasant ways that are not easy to workaround.

This reverts commit ff20982.

seancorfield added a commit that referenced this issue Jan 12, 2016

Declare bean builder syntax #417
Converted several tests to use new builder syntax to provide test
coverage. Added factory function test.

seancorfield added a commit that referenced this issue Jan 12, 2016

Remove test for ACF10 #417
Since we no longer support ACF9, we can assume isClosure() is present
without needing a version test!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment