Smoothen code base by adding dependency injection

[ghost]

I thought I had proposed something like this a year ago, but I probably never want as far as to file an issue. I propose adding an IoC-framework to Oni to enable dependency injection. Dependency injection allows fully decoupling the implementation of something from the service that defines to what requirements the implementation should adhere to. It is an invaluable tool in larger object-oriented projects, and I believe Oni to be large enough to benefit from it.

The standard dependency injection framework is InversifyJS, and with a little bit of work it should work even with React components. I think a lot of code can be made more robust by making use of it, but the exact details I have yet to figure out.

Benefits for plugin developers

Plugin developers will be able to require just about anything that they'd want to use with a set of simple decorators and some predefined types, without having to rely on global functions or factory methods. We can filter out the services we don't want to expose, and even track usage of different services in different plugins.

[justinmk]

One problem with DI (at least in Java) is that it makes call stacks less helpful. The "indirection" is quite literal, with the obvious disadvantage: now you can't directly navigate to the implementation of a service. You have to backtrack through the configuration and figure out which service the DI will resolve-to at runtime.

And what is the advantage? I'm used to DI in Java, but I actually could not tell you any big advantage.

without having to rely on global functions or factory methods

Is that really a significant advantage? DI is a constant source of debugging awkwardness. Is the cost worth this benefit?

[ghost]

@justinmk Hmm I need to think about this ... I do know that something like inversify-logger-middleware can be used to trace which instances were wired up, and I'm not entirely sure that the same problems about losing stack traces are applicable to JavaScript. It's certainly important to investigate, though.

Also, I've found this answer helpful: https://softwareengineering.stackexchange.com/a/136042/110240

[ghost]

I actually pinpointed the reason why I was so eager to add it 😅 It's because it is one of the few things that can provide an alternative to Redux while keeping the code clean. See issue #2685 for details.

[ghost]

Actually, the more I look at the code, the more I realise that DI definitely is the way to go in this project. I'm building a prototype and already managed to reduce the size of ./browser/src/App.ts by half.

[bryphe]

Thanks for thinking about this @samvv , and for the feedback @justinmk !

I thought a bit about this a loooong while ago, and while I was interested in some DI approaches, I didn't really know any robust strategies at the time for JavaScript/TypeScript.

However, I spent quite a bit of time recently poking around the VSCode codebase prototyping a way to decouple their entire extension host and hook it up for Oni 2. They have a pretty nice DI solution, documented here: https://github.com/Microsoft/vscode/wiki/Code-Organization#dependency-injection

We have a lot of places where we are bringing in a variety of 'services', with the worst cases being the actual editor implementations:

    constructor(
        private _colors: IColors,
        private _completionProviders: CompletionProviders,
        private _configuration: Configuration,
        private _diagnostics: IDiagnosticsDataSource,
        private _languageManager: LanguageManager,
        private _menuManager: MenuManager,
        private _overlayManager: OverlayManager,
        private _pluginManager: PluginManager,
        private _snippetManager: SnippetManager,
        private _themeManager: ThemeManager,
        private _tokenColors: TokenColors,
        private _workspace: Workspace,
    ) {
        super()
      ...

https://github.com/onivim/oni/blob/master/browser/src/Editor/OniEditor/OniEditor.tsx

So I think DI could be useful in simplifying that logic and boilerplate - and certainly reduce the 'wiring' we're doing in App.ts.

One problem with DI (at least in Java) is that it makes call stacks less helpful. The "indirection" is quite literal, with the obvious disadvantage: now you can't directly navigate to the implementation of a service. You have to backtrack through the configuration and figure out which service the DI will resolve-to at runtime.

This is a concern I share as well. I never tried DI in Java but I tried some things in C# (like Ninject), and I ran into these issues, too. The stack traces in C# were particularly bad when because under-the-hood it actually emitted IL that was compiled on the fly which butchered the stack traces - I believe the JS stack traces would actually be better than that case!

The code discovery aspect is important - how do I easily trace back where these instances come from? I'd be curious if the VSCode implementation addresses these issues at all, or if the benefit of removing the boilerplate / wiring in their codebase simply outweighed these disadvantages. It subjectively did make my perusal of the VSCode codebase more challenging - once I understand where the implementations came from, it was OK (and surely reduced boilerplate), but it took some time to understand the way it worked vs a more explicit strategy.

I'm open to it, though - interested to learn more about the DI approaches in TypeScript / JavaScript! @samvv - feel free to push up a branch or open a [Prototype] PR, would be cool to see how it works 👍

We could see how it impacts @justinmk 's concerns with discoverability. Would it be doable to see a sample stacktrace that bubbles up through an injected dependency?

Thanks again for the thoughts here!

[ghost]

@bryphe Thanks for the feedback! Indeed, the services are exactly the things that would benefit immensely from this approach. I'm making a prototype that combines this with issue #2685. We'll see where it leads.