Make base store non Singleton #6690

Nokel81 · 2022-12-01T14:27:47Z

resolves #4884

This PR does the following:

Removes Singleton as a super class of BaseStore
Makes sure extension API doesn't break, while still deprecating the SIngleton style static methods
Removes all the rest of the uses of Singleton that are not related to the extension API
Makes the Store migrations injectable and a dependency of the stores
Adds support for multiple runAfter's (to fix a exception in renderer)
Removes legacy global execHelm in favour of explicitly using the injectable version.

github-actions · 2022-12-02T15:33:34Z

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

github-actions · 2022-12-02T15:36:45Z

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

github-actions · 2022-12-05T14:11:34Z

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

github-actions · 2022-12-06T12:48:35Z

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

Signed-off-by: Sebastian Malton <sebastian@malton.name>

- Use it for ClusterStore Signed-off-by: Sebastian Malton <sebastian@malton.name>

Signed-off-by: Sebastian Malton <sebastian@malton.name>

- That value is not used anywhere in code Signed-off-by: Sebastian Malton <sebastian@malton.name>

Signed-off-by: Sebastian Malton <sebastian@malton.name>

src/common/app-paths/app-paths-state.injectable.ts

.../get-configuration-file-model/get-configuration-file-model.global-override-for-injectable.ts

src/common/fs/write-file-sync.injectable.ts

Iku-turso · 2022-12-14T09:54:59Z

src/test-utils/override-fs-with-fakes.ts

+      ensureDir: async (path, opts) => ensureDirSync(path, opts),
+      ensureDirSync,


I'm not sure if these should be deprecated, or even left out of fs.injectable, as using them is mostly sign of trouble, and/or the responsibility of other abstraction. WDYT?

I think you are confused because our own abstractions of writeFile, etc... use these.... so they are certainly required.

Iku-turso · 2022-12-14T09:59:20Z

.../get-configuration-file-model/get-configuration-file-model.global-override-for-injectable.ts

+    } catch {
+      // ignore
+    }


These make me uneasy. You? :)

Offtopic: such a long filename with many meanings kinda hard to perceive get-configuration-file-model.global-override-for-injectable.ts. Could we have more clear/simple (file|class?) names?

At least global-override-for-yaddayaddayadda will get shorter soon enough. Outside of that, naming should follow common sense, or what is the proposal? :)

Iku-turso · 2022-12-14T10:11:34Z

src/renderer/before-frame-starts/runnables/setup-auto-registration.injectable.ts

+        apiManager.registerApi(api);
+      };
+
+      autoRegistrationEmitter


Thought: in a sense, emitters and their listeners, such as seen here, do something that Runnable could do, but without the burden of having to make a big deal about getting listeners "enlisted". That would be the same thing as registering the injectable, which is more like a convention.

Reminder: this is a hack to preserve behaviour while making sure circular dependencies don't exist.

Iku-turso · 2022-12-14T10:15:21Z

src/renderer/before-frame-starts/runnables/setup-root-mac-class.injectable.ts

+    return {
+      id: "setup-root-mac-classname",
+      run: () => {
+        const rootElem = document.getElementById("app");


Side-effect.

By that definition, so it render. So I think your point is moot.

src/renderer/before-frame-starts/tokens.ts

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Iku-turso · 2022-12-15T12:10:38Z

src/common/fs/write-json-sync.injectable.ts

+        encoding: "utf-8",
+        spaces: 2,


These could be always set in writeJsonSync.

What do you mean? This is wrieJsonSync

ixrock · 2022-12-15T12:15:13Z

src/renderer/start-frame/start-frame.injectable.ts

    const beforeFrameStarts = runMany(beforeFrameStartsInjectionToken);
    const currentlyInClusterFrame = di.inject(currentlyInClusterFrameInjectable);

    return async () => {
-      await evenBeforeFrameStarts();
+      await beforeFrameStartsFirst();


Which frame starts? (since below we're seeing distinguishing btw cluster/main frame).
Why we should wait for? (just random question for the first time looking at the code)

Either frame, this function is here to consolidate all the initialisation steps (and the temporal dependencies between them). The reason why we split up these Runnable's (which is the type we created to describe each of these steps) into separate phases (which is what each of these runMany calls represents) is because sometimes there is a general need to have a step complete before anything else is run.

We could have instead specified a runAfter for every other runnable (which is how we specify the temporal ordering dependencies) but that seemed to be too repetitive.

Iku-turso · 2022-12-15T12:46:49Z

src/renderer/components/test-utils/get-application-builder.tsx

+    const joinPaths = mainDi.inject(joinPathsInjectable);
+    const homeDirectoryPath = mainDi.inject(homeDirectoryPathInjectable);
+
+    ensureDirSync(joinPaths(homeDirectoryPath, ".kube"));


Does this access actual filesystem or the fake one? The actual one is not to be accessed in a unit test.

This accesses the fake one because we have already overridden fsInjectable for mainDi above on line 183

src/common/runnable/run-many-for.test.ts

Signed-off-by: Sebastian Malton <sebastian@malton.name>

- https://github.com/lensapp/lens/blob/master/packages/core/src/extensions/extension-store.ts - lensapp/lens#6690 ("Make base store non Singleton") - lensapp/lens#4573 ("Elimination of global shared state") The `createInstance()` and `getInstance()` methods had been deprecated in the Lens Extensions API, so they have been eliminated from our code base also. But we're still using "global shared state" Cloud and Sync store instances. It's just that Lens isn't the one creating and holding the instance anymore, we are.

…ase (#1359) - https://github.com/lensapp/lens/blob/master/packages/core/src/extensions/extension-store.ts - lensapp/lens#6690 ("Make base store non Singleton") - lensapp/lens#4573 ("Elimination of global shared state") The `createInstance()` and `getInstance()` methods had been deprecated in the Lens Extensions API, so they have been eliminated from our code base also. But we're still using "global shared state" Cloud and Sync store instances. It's just that Lens isn't the one creating and holding the instance anymore, we are.

Nokel81 added the chore label Dec 1, 2022

Nokel81 added this to the 6.3.0 milestone Dec 1, 2022

Nokel81 requested a review from a team as a code owner December 1, 2022 14:27

Nokel81 requested review from jansav and Iku-turso and removed request for a team December 1, 2022 14:27

github-actions bot added the PR: needs rebase label Dec 2, 2022

Nokel81 force-pushed the make-BaseStore-non-singlton branch from df5117f to 06ec089 Compare December 2, 2022 15:36

github-actions bot added PR: needs rebase and removed PR: needs rebase labels Dec 2, 2022

Nokel81 force-pushed the make-BaseStore-non-singlton branch from 15744a9 to 40cb95a Compare December 6, 2022 12:48

github-actions bot removed the PR: needs rebase label Dec 6, 2022

Nokel81 added 14 commits December 6, 2022 09:18

Remove Singleton from BaseStore to remove global shared state

257082e

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Remove more usages of Singleton

9fee31a

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Replace use of legacy global execHelm with injectable

4c1e1ac

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Remove last use of legacy global execHelm

1c277a9

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Extract BaseStore deps into constructor argument

3dce6f9

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Introduce method to make store migrations injectable

ad814eb

- Use it for ClusterStore Signed-off-by: Sebastian Malton <sebastian@malton.name>

Switch HotbarStore to injectable migrations

1aa3e46

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Switch UserStore to injectable migrations

3958e61

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Move migration utils into common/utils/

6620bb5

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Switch WeblinkStore to injectable migrations

a55cd67

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Remove dead code

8462364

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Fix type error in base-store tests

a4ae7c0

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Remove tests that reference lastSeenVersion

303a397

- That value is not used anywhere in code Signed-off-by: Sebastian Malton <sebastian@malton.name>

Remove usage of legacy global .getInstance

bd47377

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Iku-turso reviewed Dec 14, 2022

View reviewed changes

src/common/app-paths/app-paths-state.injectable.ts Outdated Show resolved Hide resolved

Iku-turso reviewed Dec 14, 2022

View reviewed changes

.../get-configuration-file-model/get-configuration-file-model.global-override-for-injectable.ts Show resolved Hide resolved

Iku-turso reviewed Dec 14, 2022

View reviewed changes

src/common/fs/write-file-sync.injectable.ts Show resolved Hide resolved

Iku-turso reviewed Dec 14, 2022

View reviewed changes

ixrock reviewed Dec 14, 2022

View reviewed changes

src/renderer/before-frame-starts/tokens.ts Outdated Show resolved Hide resolved

Nokel81 added 5 commits December 14, 2022 08:34

Make startMainApplication not an injection time side effect

f43757e

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Choose better names for start-frame runnable tokens

8416d96

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Remove duplication of code in RunManyFor

7b8e97b

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Add unit tests and fix handling empty runAfter array

fcddf44

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Replace use of mobx from runManyFor with custom barrier

4dbf5c8

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Iku-turso reviewed Dec 15, 2022

View reviewed changes

ixrock reviewed Dec 15, 2022

View reviewed changes

Iku-turso reviewed Dec 15, 2022

View reviewed changes

src/common/runnable/run-many-for.test.ts Show resolved Hide resolved

Add missing test

c06430f

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Nokel81 mentioned this pull request Dec 15, 2022

Renderer logging to file #6740

Closed

Iku-turso approved these changes Dec 15, 2022

View reviewed changes

Iku-turso merged commit 25f37ac into master Dec 15, 2022

Iku-turso deleted the make-BaseStore-non-singlton branch December 15, 2022 15:07

This was referenced Dec 15, 2022

Fix typing of ExtensionStore.createInstance static #6764

Merged

Release 6.3.0 #6804

Merged

This was referenced Dec 27, 2022

Blank cluster screen when running a build from source (6.3) #6829

Closed

Allow main send messages to cluster frames #6835

Merged

aleksfront mentioned this pull request Dec 28, 2022

Fix kubeconfig sync not saving data #6837

Merged

stefcameron mentioned this pull request Mar 28, 2023

ExtensionStore is no longer a Singleton in Lens since some 6.4.x release Mirantis/lens-extension-cc#1359

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make base store non Singleton #6690

Make base store non Singleton #6690

Nokel81 commented Dec 1, 2022

github-actions bot commented Dec 2, 2022

github-actions bot commented Dec 2, 2022

github-actions bot commented Dec 5, 2022

github-actions bot commented Dec 6, 2022

Iku-turso Dec 14, 2022

Nokel81 Dec 14, 2022

Iku-turso Dec 14, 2022

ixrock Dec 14, 2022

Iku-turso Dec 15, 2022

Iku-turso Dec 14, 2022

Nokel81 Dec 14, 2022

Iku-turso Dec 14, 2022

Nokel81 Dec 14, 2022

Iku-turso Dec 15, 2022

Nokel81 Dec 15, 2022

ixrock Dec 15, 2022

Nokel81 Dec 15, 2022

Iku-turso Dec 15, 2022

Nokel81 Dec 15, 2022

		ensureDir: async (path, opts) => ensureDirSync(path, opts),
		ensureDirSync,

Make base store non Singleton #6690

Make base store non Singleton #6690

Conversation

Nokel81 commented Dec 1, 2022

github-actions bot commented Dec 2, 2022

github-actions bot commented Dec 2, 2022

github-actions bot commented Dec 5, 2022

github-actions bot commented Dec 6, 2022

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