razor icon indicating copy to clipboard operation
razor copied to clipboard

Don't use service locator pattern where it's not required

Open ryanbrandenburg opened this issue 3 years ago • 1 comments

Roslyn makes extensive use of the "service locator pattern" for their DI systems. That seems to work well for them (and has reportedly had significant performance benefits), but it is not recommended in general.

When I moved Razor to CLaSP fresh off my work in the Roslyn repo I proliferated this pattern in many placed I touched. Let's remove those usages which aren't required (say for a self-destructing service or one which is not available when the class is constructed). They should all be off of either requestContext.GetRequiredService or requestContext.LspServices. Then we can consider adding a deprecated attribute to these properties to avoid their over-use.

ryanbrandenburg avatar Oct 05 '22 23:10 ryanbrandenburg

💯

AFAIK roslyn only does this for scoped services (ie, services specific to a workspace instance, or language for the C#/VB split). For us, where everything (I think) is singleton, I much prefer being declarative about it, and using constructors.

davidwengier avatar Oct 06 '22 00:10 davidwengier

Moving to backlog. I audited the usages of this, and whilst it's still good to do, the debt is all in areas that will be removed eventually with cohosting anyway.

davidwengier avatar Jun 20 '24 07:06 davidwengier