Remove MenuRegistry in favour of dependency injection #4527

jansav · 2021-12-08T11:21:21Z

What we did

We removed a typical registry MenuRegistry and replaced it with an injectable ElectronMenuItems. The injectable instantiates a MobX-computed observed from all applicable extensions. With this, we propose dependency injection over shared global state, and reactivity over statefulness.
With removal of MenuRegistry, also an instance of Singleton-base-class got removed. With this, we also limit shared global state.
Now that there is friendlier unit-testability, we added unit tests for said ElectronMenuItems.
We made sure that “Extension API” remains unmolested.
We made injectables usable in Main process similar to Renderer.

Rationale

We recognized implementations of base-classes Singleton and BaseRegistry as main causes of problems like code rigidity, non-testability, tendency to bug etc. These classes describe shared global state, logical dependencies, and statefulness over reactivity. Now that we have infrastructure in place to inject dependencies (i.e. injectable), we can replace these anti-patterns with dependency injection. It is also handy that outsourcing lifecycles (e.g. singleton) to DI comes naturally, without the dependee even knowing about it.

What’s next:

We will keep applying this solution to other registries for same benefits.

Closes #4475.

…d local state Signed-off-by: Janne Savolainen <janne.savolainen@live.fi> Co-authored-by: Mikko Aspiala <mikko.aspiala@gmail.com>

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

jansav · 2021-12-08T11:27:17Z

src/main/index.ts

  onQuitCleanup.push(
-    initMenu(windowManager),
+    initMenu(windowManager, menuItems),


This would be cleaner if function initMenu was injectable. @panuhorsmalahti would you like to do this with us? :)

I guess, what is the benefit of this being injectable here? Instead of just computing the entries?

This file shouldn’t need to know what are the dependencies for initMenu. Simplicity.

I meant, why is menuItems not just a computed without this injection

Same argument:

menuItems has dependencies that I shouldn’t need to know when I use it.

All this code should care is that the required interface is somehow fulfilled.

@jansav I'm afraid I'm a bit busy with other Lens 🔥 issues for the time being..

No worries @panuhorsmalahti :)

We can share you the result, if you want.

Sure, that sounds interesting.

I do believe the promised result will satisfy @Nokel81, as it will extract responsibilities related to instantiation away from this file, where it doesn't quite belong. @Nokel81 how about you, wanna have a go doing this with us? It may be trivial here, but having this freedom somewhere else will be a big deal.

Or the beer's on me ;)

jansav · 2021-12-08T11:29:19Z

src/extensions/extension-loader.ts

-
-      return removeItems;
-    });
+    this.autoInitExtensions(() => Promise.resolve([]));
  }


Notice how we no longer need to dispose items from a state when extension is disabled or removed.

Yes I have wanted to do this for some time, I even made #2245 about 10 months ago, but no one wanted to me to pursue it.

Glad to see that you have had same kind of changes in mind. However, I don’t see a reason not to pursue simplicity that reactive programming gives. Current state is less than optimal.

src/main/getDiForUnitTesting.ts

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Nokel81 · 2021-12-08T17:10:31Z

src/main/index.ts

@@ -61,11 +61,15 @@ import { FilesystemProvisionerStore } from "./extension-filesystem";
 import { SentryInit } from "../common/sentry";
 import { ensureDir } from "fs-extra";
 import { Router } from "./router";
-import { initMenu } from "./menu";
+import { initMenu } from "./menu/menu";


why change this import?

Moved file to separate directory. Prefer not to create index.ts when really not needed.

Okay I guess, I find index files much cleaner to read especially since the rest of this repo we use them.

We have seen both used in this repo. We prefer it this way so that we don’t need so much boilerplate code.

Not that big deal though for us

To me it seems that more common practice is to use "index" in this repo but that has a downside because it requires tree-shaking (which might not work in practice too well). Without tree-shaking we are actually "loading" all dependencies from index ... of course import shouldn't have side-effects in a perfect world (we are not there yet).

Hmm. The index-files do feel like an unnecessary indirection and complication. We could consider if the benefits of encapsulation really overweigh them. As said, not a bid deal, as there's far bigger fish out there :)

...but in general, when it comes to side-effects on import, I'm afraid it's not something our team should tolerate at all. Let's make a hard line for this, right guys? :)

I agree, it also makes circular dependencies too easy

Nokel81 · 2021-12-08T17:18:02Z

src/main/index.ts

 import { initTray } from "./tray";
 import { kubeApiRequest, shellApiRequest, ShellRequestAuthenticator } from "./proxy-functions";
 import { AppPaths } from "../common/app-paths";
 import { ShellSession } from "./shell-session/shell-session";
+import { getDi } from "./getDi";
+import electronMenuItemsInjectable from "./menu/electron-menu-items.injectable";


why is this "electron" menu items?

Since menuItems is not enough to make it obvious. Other option was appMenuItems but that has same effect. App can be anything. Using Electron in name is not best, but still lot better than without it. Maybe mainAppMenuItems?

Nokel81 · 2021-12-08T17:20:06Z

src/extensions/extensions.injectable.ts

+  { extensionLoader: ExtensionLoader }
+> = {
+  getDependencies: () => ({
+    extensionLoader: ExtensionLoader.createInstance(),


No you aren't supposed to create the instances in more than once place.

Yep, this is the issue with current Singleton. I shouldn’t need to know how and when instance is created.

If we change this to .getInstance() we need to be sure that .createInstance() has been called.

This is something that we need to fix by getting rid of Singleton

I completely disagree, just because something is a singleton doesn't mean there aren't dependency's on other instances, and those instances might need arguments to be created.

Solution is to use DI to make instances. That’s how I don’t need to worry about creating instance and getting arguments somewhere.

In this case, it’s clear that ExtensionLoader needs to be injected. Therefore injectable should be created.

I think @Nokel81 means that we shouldn't call ExtensionLoader.createInstance() in multiple places (see main/index.ts for other call). That said, it's a bit unfortunate that caller needs to know anything about this -> maybe instance should always come via injectable.

I know and I agree.

To be honest, I’m not sure why it was left as .createInstance() since .getInstance() is more than enough.

However it works as a great example of an issue caused Singleton and permits this kind discussion.

I’ll change it tomorrow anyway :)

Because not every place that consumes an instance needs/should know how to create it.

This we know ;)

Nokel81 · 2021-12-08T17:21:06Z

src/extensions/extension-loader.ts

@@ -47,7 +46,7 @@ const logModule = "[EXTENSIONS-LOADER]";
 */
 export class ExtensionLoader extends Singleton {
  protected extensions = observable.map<LensExtensionId, InstalledExtension>();
-  protected instances = observable.map<LensExtensionId, LensExtension>();
+  instances = observable.map<LensExtensionId, LensExtension>();


I don't think this should be public. A getter for the list of instances could be useful if it doesn't already exist.

Agreed, this being exposed for external mutation is scary. Good catch.

Nokel81 · 2021-12-08T17:26:08Z

src/main/menu/electron-menu-items.test.ts

@@ -0,0 +1,132 @@
+/**


Please put test files under a __test__ folder

Why? There are tests already in codebase that are not in __tests__ directory.

I'd argue for having tests in a tests folder (being a bit vague to not bikeshed too much). If Lens codebase is not consistent wrt, we should strive to make it so. I think file navigation becomes less convenient if test files are located next to application code.

You might be both right:

Sometimes unit tests specify real life behaviour instead of workings of certain technical entrypoint (i.e. a file). For this, __tests__makes sense, as the entrypoint becomes less relevant. This does not happen in current codebase yet. Some call this integration testing or functional testing, but let's not get into semantics.

At other times unit tests specify workings of a technical detail. For this, placing the test file next to entrypoint of unit makes sense, as the presence of tests (or lack thereof) is a valuable piece of information not to be swept away in a directory. When doing this, inconvenience in navigation is oftentimes sign of another problem, namely files not being organised in directories properly. Tests should not make directories feel cluttered.

Nokel81 · 2021-12-08T17:26:27Z

src/main/menu/electron-menu-items.injectable.ts

+  }),
+
+  instantiate: ({ extensions }) =>
+    computed(() => extensions.get().flatMap(extension => extension.appMenus)),


what about disabled extensions?

Just noticed that there is something weird happening here. Disabled extensions work already, but it seems that is not enough. We need to be sure that .init() has been called for extension as well.

I have required changes done already and pushing those here tomorrow.

Hmm. In general, with DI we will have more guarantees that temporal requirements, such as .init() this or .createInstance() that, will have been called when expected. Good.

Nokel81 · 2021-12-08T17:27:10Z

src/main/index.ts

  onQuitCleanup.push(
-    initMenu(windowManager),
+    initMenu(windowManager, menuItems),


I guess, what is the benefit of this being injectable here? Instead of just computing the entries?

Nokel81 · 2021-12-08T17:27:33Z

src/main/menu/menu-registration.d.ts

@@ -18,15 +18,8 @@
 * IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
 * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
 */
-
-// Extensions API -> Global menu customizations


Why remove a comment?

It serves no purpose -> dead weight.

Nokel81 · 2021-12-08T17:27:51Z

src/main/menu/menu-registration.d.ts

@@ -18,15 +18,8 @@
 * IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN


Also, why move this file?

This was located in a file which was deleted.

Ah well the git diff implied that it was moved here instead. Then why delete that file in registries instead of just deleting the registry?

Would be silly to leave empty file there? (Or file that doesn’t do anything and doesn’t have anything to do with registries.)

Before it was the place that this interface was used first time, but since that doesn’t exist anymore, it makes sense to move it closer to useplace.

It is also used in LensMainExtension and I would argue that is its "first use" and this is merely its consumer.

I would say that interface MenuRegistration exists for making sure that Menu has items with proper interface. That being said, Menu is the one that defines the requirements for the interface -> logical place for the interface is next to Menu.

Nokel81 · 2021-12-08T17:30:47Z

src/main/menu/menu.ts

-  for (const { parentId, ...menuItem } of MenuRegistry.getInstance().getItems()) {
-    if (!appMenu.has(parentId)) {
-      logger.error(`[MENU]: cannot register menu item for parentId=${parentId}, parent item doesn't exist`, { menuItem });
+  for (const menuItem of electronMenuItems) {


IMO it is cleaner to not pass menuItem.parentId to the list of submenus.

Hmm. I suppose I considered the complication in typing not worth this. Do you agree? I see your point of encapsulation, but this case is trivial, and therefore simplicity is more pragmatic.

How is the new code more simple?

I don't see this issue as good time investment. I can change it back if you feel strongly about it :)

…as enabled extensions Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

…h no changes required on use places Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

…ld be created Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com> Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>

github-actions · 2021-12-14T17:02:32Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

…lass with no changes required on use places" This reverts commit 34a47f5 Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

…y-obsolete Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

github-actions · 2021-12-15T14:33:33Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

Iku-turso · 2021-12-15T14:45:17Z

Alright! With the latest changes, this will be one of those bigger PR's we hinted the team about. The big push was getting ExtensionLoader to comply with dependency injection, and more importantly the design choices which made that possible.

Here's our sincerest hopes to fast-track this PR, as it will directly support the next cool stuff to come. Onwards!

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

jansav · 2021-12-16T06:21:44Z

@Nokel81, I fixed failing tests and linting errors. Needs to be approved again.

jansav and others added 2 commits December 8, 2021 13:10

Switch to getting menu items by observing change in extensions instea…

67c24ca

…d local state Signed-off-by: Janne Savolainen <janne.savolainen@live.fi> Co-authored-by: Mikko Aspiala <mikko.aspiala@gmail.com>

Remove obsolete code

717b62d

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

jansav added the enhancement New feature or request label Dec 8, 2021

jansav requested review from jakolehm and panuhorsmalahti December 8, 2021 11:21

jansav requested a review from a team as a code owner December 8, 2021 11:21

jansav requested review from jweak and removed request for a team December 8, 2021 11:21

jansav commented Dec 8, 2021

View reviewed changes

jweak reviewed Dec 8, 2021

View reviewed changes

src/main/getDiForUnitTesting.ts Outdated Show resolved Hide resolved

Iku-turso reviewed Dec 8, 2021

View reviewed changes

src/main/getDiForUnitTesting.ts Outdated Show resolved Hide resolved

Remove forgotten console.log used for debug

41837e8

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

jansav force-pushed the make-menu-registry-obsolete branch from 1c7d29a to 41837e8 Compare December 8, 2021 12:17

Nokel81 reviewed Dec 8, 2021

View reviewed changes

jansav and others added 4 commits December 9, 2021 07:41

Make sure that extensions have been initialized before exposing them …

f9cf08b

…as enabled extensions Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Introduce a way to start replacing usages of Singleton base-class wit…

34a47f5

…h no changes required on use places Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Make ExtensionLoader injectable to avoid confusion when instance shou…

b4a1c2a

…ld be created Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Fix hangup on application start by auto-registering more injectables

f3d45ea

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com> Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>

github-actions bot added the PR: needs rebase label Dec 14, 2021

jansav mentioned this pull request Dec 15, 2021

Elimination of global shared state #4573

Closed

jansav added 4 commits December 15, 2021 15:13

Adapt to change in DI to minimize boilerplate

0b172de

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Replace global state usages of ExtensionLoader with DI

c92c8f8

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Revert "Introduce a way to start replacing usages of Singleton base-c…

9f7515d

…lass with no changes required on use places" This reverts commit 34a47f5 Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Merge remote-tracking branch 'upstream/master' into make-menu-registr…

2189679

…y-obsolete Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

github-actions bot removed the PR: needs rebase label Dec 15, 2021

Nokel81 changed the title ~~Replace a registry with an injectable for benefits~~ Remove MenuRegistry in favour of dependency injection Dec 15, 2021

Nokel81 previously approved these changes Dec 15, 2021

View reviewed changes

Nokel81 added chore and removed enhancement New feature or request labels Dec 15, 2021

Nokel81 added this to the 5.4.0 milestone Dec 15, 2021

jansav dismissed Nokel81’s stale review via 2c13b5d December 16, 2021 05:11

jansav added 3 commits December 16, 2021 07:14

Fix linting

a2486da

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Revert code style change

250b301

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Fix test that was accidentally broken

8337dfd

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

jansav force-pushed the make-menu-registry-obsolete branch from 2c13b5d to 8337dfd Compare December 16, 2021 05:14

jansav requested review from Nokel81 and Iku-turso and removed request for jakolehm and panuhorsmalahti December 16, 2021 05:16

Iku-turso approved these changes Dec 16, 2021

View reviewed changes

Nokel81 approved these changes Dec 16, 2021

View reviewed changes

jansav merged commit 3418c0a into lensapp:master Dec 16, 2021

jim-docker mentioned this pull request Feb 1, 2022

release v5.4.0-alpha.0 #4780

Merged

		@@ -18,15 +18,8 @@
		* IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN

Remove MenuRegistry in favour of dependency injection #4527

Remove MenuRegistry in favour of dependency injection #4527

Conversation

jansav commented Dec 8, 2021

What we did

Rationale

What’s next:

jansav Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jansav Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jansav Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jansav Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 14, 2021

github-actions bot commented Dec 15, 2021

Iku-turso commented Dec 15, 2021

jansav commented Dec 16, 2021

jansav Dec 8, 2021 •

edited

Loading

jansav Dec 8, 2021 •

edited

Loading

jansav Dec 8, 2021 •

edited

Loading

jansav Dec 8, 2021 •

edited

Loading