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

Move ExecutionContextScopingLifestyle and LifetimeScopeLifestyle into the core library. #299

Closed
dotnetjunkie opened this Issue Sep 27, 2016 · 12 comments

Comments

3 participants
@dotnetjunkie
Collaborator

dotnetjunkie commented Sep 27, 2016

Doing so will cause Simple Injector to become incompatible with .NET 4.0. Since .NET 4.0 is becoming more and more obsolete, this will unlikely be a problem. This might however be a problem for older platforms that don't support AsyncLocal or CallContext; we either need to make version of the core library without an ExecutionContextScopingLifestyle; or we need to drop support for those platforms.

We should also consider renaming the lifestyles. Suggestion:

  • AsyncScopingLifestyle
  • ThreadScopingLifestyle

@dotnetjunkie dotnetjunkie added this to the v4 milestone Sep 27, 2016

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Sep 29, 2016

Moving ExecutionContextScopingLifestyle into the core library basically means the core library will be >= .NETStandard1.3 while v3.2 is >= .NETStandard1.0. This means that the Core library can only work on .NET 4.5 and up and .NETStandard1.3 and up. Of course we can still have multiple builds and this would allow a special .NET 4.0 build (as there is for v3) that omits the AsyncScopeLifestyle.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Oct 19, 2016

Effect of merging those lifestyles into the Core library is that the BeginLifetimeScope() and BeginExecutionContextScope() extension methods will always be available. This might be confusing for the user. Which method to call?

So instead, we might think about moving these methods to the ScopedLifestyle class instead. For instance, we could force the user to do the following:

using (Lifestyle.Scoped.BeginScope(container))
{
    container.GetInstance<X>();
}
@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Nov 18, 2016

Here are some possible names for the name lifestyle:

  • AsyncScopingLifestyle
  • AsyncScopeLifestyle
  • AsyncScopedLifestyle

What would be the best name?

Things to consider:

  • The 'old' name is ExecutionContextScopeLifestyle
  • The base class is called ScopedLifestyle.
@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Dec 1, 2016

Should we simplify working with the AsyncScopedLifestyle and ThreadScopedLifestyle (which will be part of the core library in v4) by adding shortcuts to them from the Lifestyle class, just as we now do with Lifestyle.Transient, Lifestyle.Scoped and Lifestyle.Singleton?

This would mean typical usage would become:

var container = new Container();
container.Options.DefaultScopedLifestyle = Lifestyle.AsyncScoped;

container.Register<MyClass>(Lifestyle.Scoped);

Instead of:

var container = new Container();
container.Options.DefaultScopedLifestyle = new SimpleInjector.Lifestyles.AsyncScopedLifestyle();

container.Register<MyClass>(Lifestyle.Scoped);

I can imagine adding Lifestyle.AsyncScoped and Lifestyle.ThreadScoped would be confusing next to the Lifestyle.Scoped:

  • Lifestyle.AsyncScoped
  • Lifestyle.ThreadScoped
  • Lifestyle.Transient
  • Lifestyle.Scoped
  • Lifestyle.Singleton
@qujck

This comment has been minimized.

Collaborator

qujck commented Dec 1, 2016

The confusion is more with Scoped, should it have been DefaultScoped and if so is it worth a breaking change now?

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Dec 1, 2016

Although Lifestyle.DefaultScoped does make it clearer that there is a relationship between Lifestyle.Scoped and ContainerOptions.DefaultScopedLifestyle, I do think that the Default prefix clutters the registration of developers:

container.Register<IRepository<Person>, PersonRepository>(Lifestyle.DefaultScoped);
container.Register<IRepository<Order>, OrderRepository>(Lifestyle.DefaultScoped);
container.Register<IRepository<Item>, ItemRepository>(Lifestyle.DefaultScoped);
container.Register<IRepository<Product>, ProductRepository>(Lifestyle.DefaultScoped);
@qujck

This comment has been minimized.

Collaborator

qujck commented Dec 1, 2016

Agreed, what about just Default?

container.Register<IRepository<Person>, PersonRepository>(Lifestyle.Default);
container.Register<IRepository<Order>, OrderRepository>(Lifestyle.Default);
container.Register<IRepository<Item>, ItemRepository>(Lifestyle.Default);
container.Register<IRepository<Product>, ProductRepository>(Lifestyle.Default);
@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Dec 1, 2016

I don't think it is obvious that the Lifestyle.Default would map to Options.DefaultScopedLifestyle. I would rather think it would map to Options.DefaultLifestyle (which is new in v3.3).

I don't mind making (yet another) breaking change in v4, but since this is a really prominent part of the API, the improvement should be rather significant. I'm currently not sure whether renaming Lifestyle.Scoped is a big enough improvement that would justify this. That would probably mean we shouldn't add Lifestyle.AsyncScoped and Lifestyle.ThreadScoped, since they cause confusion.

@qujck

This comment has been minimized.

Collaborator

qujck commented Dec 1, 2016

What about this:

  • Lifestyle.AsyncScoped
  • Lifestyle.ThreadScoped
  • Lifestyle.Transient
  • Lifestyle.DefaultScope
  • Lifestyle.Singleton

I'm not a fan of Scoped regardless of the other options, I don't believe it clearly describes what the property holds, whereas DefaultScope does. But, as you say, the api is uglier with container.Register<IRepository<Product>, ProductRepository>(Lifestyle.DefaultScope);.

I think that container.RegisterDefaultLifestyle<IRepository<Product>, ProductRepository>(); is neater than both the other options as the full intention of the registration is in the registration method name rather than tucked away as a parameter - but I understand that this suddenly brings a load more RegisterXXX methods to the party.

Not the easiest decision to make! None of these options are simple.

@qujck

This comment has been minimized.

Collaborator

qujck commented Dec 1, 2016

why Options.DefaultScopedLifestyle and Options.DefaultLifestyle?

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Dec 1, 2016

why Options.DefaultScopedLifestyle and Options.DefaultLifestyle?

See: #313

@TheBigRic

This comment has been minimized.

Collaborator

TheBigRic commented Dec 1, 2016

Maybe this comment is too late in the thread but I think this:

container.Options.DefaultScopedLifestyle = Lifestyle.AsyncScoped;

is pretty strange. Because this:

container.Options.DefaultScopedLifestyle = Lifestyle.Scoped;

will throw an exception because it will cause a StackOverflowException otherwise (if I'm not mistaken).

So this will be confusing. Although Lifestyle.xxx is not an enum it almost acts like one and making 2 out of 3 behave differently than others is just hard to grasp.

What about:

// these options could be default if not set by the user
// the user will in that case have the option to overwrite
// the various integrations packages could override these with
// the integration package specific ones like WebRequestLifestyle
container.Options.DefaultLifestyle = new TransienstLifestyle();
container.Options.DefaultScopedLifestyle = new ThreadScopedLifestyle();
container.Options.DefaultAsyncScopedLifestyle = new AsyncScopedLifestyle();

container.Register<ITransientService, Service1>();
container.Register<ILifetimeScopeService, Service2>(Lifestyle.Scoped);
container.Register<IAsyncScopeService, Service2>(Lifestyle.AsyncScoped);

dotnetjunkie added a commit that referenced this issue Dec 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment