Remove `BaseStore<T>` and replace with composition of dependencies #7002

Nokel81 · 2023-01-24T15:41:58Z

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

github-actions · 2023-01-24T19:34:44Z

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

github-actions · 2023-01-24T20:59:19Z

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

github-actions · 2023-01-27T15:36:57Z

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

github-actions · 2023-01-27T15:39:33Z

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

ixrock

LGTM

ixrock · 2023-01-27T20:14:06Z

packages/core/src/common/entity-preferences/store.ts

+    runInAction(() => {
+      this.preferences.set(entityId, merge(this.preferences.get(entityId), preferences));
+    });


Why do we need to "run in action" that single observable's update? or updating any map/set or single in general observable unit. Afaik runInAction needs to optimize update something/optimize reactions/etc? and do it in single "transaction" when updates multiple observables within single function scope, e.g.

// that would make sense for me for example runInAction(() => { this.preferences.set(entityId, merge(this.preferences.get(entityId), preferences)); this.somethingObservableToo = "UPDATED!"; });

Yes runInAction is a way to specify a transaction. The reason why updating a single observable doesn't normally need to also be within an action is because we currently have enforceActions: false as part of our mobx configuration.

...c/extensions/extension-loader/file-system-provisioner-store/file-system-provisioner-store.ts

packages/core/src/extensions/extensions-store/extensions-store.ts

ixrock · 2023-01-27T20:25:17Z

packages/core/src/main/cluster/manager.ts

@@ -246,7 +246,7 @@ export class ClusterManager {

  protected onNetworkOnline = () => {
    this.dependencies.logger.info(`${logPrefix} network is online`);
-    this.dependencies.store.clustersList.forEach((cluster) => {
+    this.dependencies.store.clustersList.get().forEach((cluster) => {


nit: why not? 🤪
anyway.. i don't think there is a big need to convert normal observable fields to computed values on the way (too much).

Suggested change

this.dependencies.store.clustersList.get().forEach((cluster) => {

this.dependencies.get().store.get().clustersList.get().forEach((cluster) => {

github-actions · 2023-02-01T20:40:06Z

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

github-actions · 2023-02-01T20:45:48Z

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

github-actions · 2023-02-15T17:20:36Z

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

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

- To fix type errors Signed-off-by: Sebastian Malton <sebastian@malton.name>

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

- Ubuntu CI seems to format arrays in snapshots differently than macOS locally Signed-off-by: Sebastian Malton <sebastian@malton.name>

Iku-turso

Live-reviewed

Nokel81 added the chore label Jan 24, 2023

Nokel81 added this to the 6.4.0 milestone Jan 24, 2023

Nokel81 requested a review from a team as a code owner January 24, 2023 15:41

Nokel81 requested review from ixrock and aleksfront and removed request for a team January 24, 2023 15:41

Nokel81 force-pushed the issue-3498 branch from 00ef6f4 to e702b66 Compare January 24, 2023 19:34

github-actions bot added the PR: needs rebase label Jan 24, 2023

Nokel81 force-pushed the base-store-composition branch from e2d9726 to a575644 Compare January 24, 2023 20:59

github-actions bot removed the PR: needs rebase label Jan 24, 2023

Nokel81 modified the milestones: 6.4.0, 6.5.0 Jan 26, 2023

Nokel81 changed the title ~~Convert BaseStore<T> away from abstract class to simplify dependency structure~~ Convert BaseStore<T> away from abstract class to simplify dependency structure Jan 27, 2023

github-actions bot added the PR: needs rebase label Jan 27, 2023

Nokel81 force-pushed the base-store-composition branch from a575644 to 1e3d048 Compare January 27, 2023 15:39

github-actions bot removed the PR: needs rebase label Jan 27, 2023

ixrock previously approved these changes Jan 27, 2023

View reviewed changes

github-actions bot added the PR: needs rebase label Feb 1, 2023

Nokel81 force-pushed the base-store-composition branch from 05191a0 to 0e52f3d Compare February 1, 2023 20:45

github-actions bot removed the PR: needs rebase label Feb 1, 2023

github-actions bot added the PR: needs rebase label Feb 15, 2023

Nokel81 force-pushed the base-store-composition branch from 0e52f3d to af5bc6b Compare February 15, 2023 19:22

github-actions bot removed the PR: needs rebase label Feb 15, 2023

Nokel81 added 16 commits March 22, 2023 15:50

Fully split apart the enabled extensions storage

12e4388

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

Fully split apart the clusters storage

f1f8e19

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

Fully split apart the hotbar storage

ae32375

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

Fully split apart the weblinks storage

037c62d

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

Fully split apart the user preferences storage

4055732

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

Fix crashing

a446a25

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

Fix tests and snapshots

34dafcd

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

Fix integration test failures

83b72f1

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

Improve typing to prevent errors in the future.

ea7a24d

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

Cleanup @k8slens/messaging and friends

a445fdd

- To fix type errors Signed-off-by: Sebastian Malton <sebastian@malton.name>

Fix lint issue

cb538bf

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

Fix type errors

b5178c7

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

Fix global override not being complete enough causing tests to fail

ff1c176

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

Bump memory for unit tests on CI

6fba195

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

Attempt to fix memory issue on CI again

eb5f660

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

Fixup test because of new injectables

6e65fda

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

Nokel81 force-pushed the base-store-composition branch from e44f44d to 6e65fda Compare March 22, 2023 19:57

Nokel81 added 7 commits March 24, 2023 14:07

Upgrade Jest

af46ca7

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

Fix unit tests falling over

0989278

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

Back out jest config change

2845df8

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

Remove console log

93c309d

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

Update snapshot

b18efcc

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

Fix tests by matching equality instead of snapshots

81b2179

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

Fix tests by forcing specific snapshot style

7f9fc5c

- Ubuntu CI seems to format arrays in snapshots differently than macOS locally Signed-off-by: Sebastian Malton <sebastian@malton.name>

Nokel81 changed the title ~~Convert BaseStore<T> away from abstract class to simplify dependency structure~~ Remove BaseStore<T> and replace with composition of dependencies Mar 28, 2023

Iku-turso approved these changes Mar 28, 2023

View reviewed changes

Iku-turso merged commit 49f0a1a into master Mar 28, 2023

Iku-turso deleted the base-store-composition branch March 28, 2023 14:54

jweak mentioned this pull request Apr 3, 2023

Hidden metrics preferences do not update over to cluster frame #7461

Open

Nokel81 mentioned this pull request Apr 12, 2023

Release 6.5.0-alpha.4 #7535

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `BaseStore<T>` and replace with composition of dependencies #7002

Remove `BaseStore<T>` and replace with composition of dependencies #7002

Nokel81 commented Jan 24, 2023 •

edited

Loading

github-actions bot commented Jan 24, 2023

github-actions bot commented Jan 24, 2023

github-actions bot commented Jan 27, 2023

github-actions bot commented Jan 27, 2023

ixrock left a comment

ixrock Jan 27, 2023 •

edited

Loading

Nokel81 Jan 27, 2023

ixrock Jan 27, 2023

github-actions bot commented Feb 1, 2023

github-actions bot commented Feb 1, 2023

github-actions bot commented Feb 15, 2023

Iku-turso left a comment

	this.dependencies.store.clustersList.get().forEach((cluster) => {
	this.dependencies.get().store.get().clustersList.get().forEach((cluster) => {

Remove BaseStore<T> and replace with composition of dependencies #7002

Remove BaseStore<T> and replace with composition of dependencies #7002

Conversation

Nokel81 commented Jan 24, 2023 • edited Loading

github-actions bot commented Jan 24, 2023

github-actions bot commented Jan 24, 2023

github-actions bot commented Jan 27, 2023

github-actions bot commented Jan 27, 2023

ixrock left a comment

Choose a reason for hiding this comment

ixrock Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

Nokel81 Jan 27, 2023

Choose a reason for hiding this comment

ixrock Jan 27, 2023

Choose a reason for hiding this comment

github-actions bot commented Feb 1, 2023

github-actions bot commented Feb 1, 2023

github-actions bot commented Feb 15, 2023

Iku-turso left a comment

Choose a reason for hiding this comment

Remove `BaseStore<T>` and replace with composition of dependencies #7002

Remove `BaseStore<T>` and replace with composition of dependencies #7002

Nokel81 commented Jan 24, 2023 •

edited

Loading

ixrock Jan 27, 2023 •

edited

Loading