chore(databases-collection): refactor create db / create coll to a single create namespace plugin COMPASS-7410 #5100

gribnoysup · 2023-11-10T17:33:39Z

Last follow-up to #5085

Merges create database and create collection plugin into a single create namespace plugin
Store is updated to typescript, but all existing logic is preserved with minimal changes
Plugin is using service injectors for everything, including the new instance locator

…ngle create namespace plugin

gribnoysup · 2023-11-10T17:37:15Z

packages/compass-app-stores/src/provider.ts

@@ -3,7 +3,7 @@ import { createContext, useContext } from 'react';

 export const InstanceContext = createContext<MongoDBInstance | null>(null);

-export const useMongoDBInstance = (): MongoDBInstance => {
+export const mongoDBInstanceLocator = (): MongoDBInstance => {


@addaleax don't know if you have a strong preference here, I thought that I'd rename it to match other methods we have and to indicate a bit better that it's not supposed to be used as a hook in React (even though it sorta is).

I think if we want to add actual hooks here (should be very valuable in some cases!), those should probably be a bit more high level, encapsulating the model subscription logic for a required field internally instead of just exposing the modal to the React view 🤔

sure, no strong preferences here! happy to stick to whatever pattern we choose 🙂

addaleax · 2023-11-10T20:39:55Z

packages/instance-model/index.d.ts

@@ -100,6 +100,7 @@ declare class MongoDBInstance extends MongoDBInstanceProps {
  }): Promise<Collection | null>;
  removeAllListeners(): void;
  on(evt: string, fn: (...args: any) => void);
+  off(evt: string, fn: (...args: any) => void);


lol 😄 https://github.com/mongodb-js/compass/pull/5102/files#diff-a79e9b0455facd1015dd43ba2327be7c140c049fcbb16ef624e15a67dfc38cbaR103 no strong feelings either way will align that with this PR ;)

ah actually, now I remember why I did this; AppRegistry has removeListener but not off. Should we also add off to AppRegistry then?

We can do that for consistency, yes, especially now that we use it so much! With ampersand models you have off, but not removeListener, so this is why I added it here and it'd be harder for us to extend ampersand models to support removeListener compared to doing this for app registry 🙂

Ahh, wait, I think I'm just silly, I'm sure I got "can't call undefined error" when trying removeListener, but it's definitely there. Will update!

Renamed to align with your PR in 6691588

…ice mock helper; add new plugin tests

gribnoysup · 2023-11-13T11:12:26Z

Added tests, should be ready for review now

gribnoysup · 2023-11-13T11:17:25Z

packages/hadron-app-registry/src/register-plugin.tsx

+  /**
+   * Convenience method for testing: allows to override services and app
+   * registries available in the plugin context
+   *
+   * @example
+   * const PluginWithLogger = registerHadronPlugin({ ... }, { logger: loggerLocator });
+   *
+   * const MockPlugin = PluginWithLogger.withMockServices({ logger: Sinon.stub() });
+   *
+   * @param mocks Overrides for the services locator values and registries
+   *              passed to the plugin in runtime. When `globalAppRegistry`
+   *              override is passed, it will be also used for the
+   *              localAppRegistry override unless `localAppRegistry` is also
+   *              explicitly passed as a mock service.
+   */
+  withMockServices(
+    mocks: Partial<Registries & Services<S>>
+  ): React.FunctionComponent<T>;


@addaleax this is this new testing helper I mentioned, I also switched drop-namespace tests to use it too (although due to how this particular plugin works, it doesn't make that much of a difference in that case)

addaleax · 2023-11-13T11:49:53Z

packages/hadron-app-registry/src/register-plugin.tsx

+            return [s, mockServices[s] ? () => mockServices[s] : services?.[s]];
+          })
+        ) as unknown as S;
+        const MockPlugin = registerHadronPlugin(config, mockServiceLocators);


Won’t it be an issue that this re-uses the same registryName as the original plugin and therefore shares a store (which may have already been initialized with the original services)?

Hmmm, I think for that to conflict you'd need to actually render / activate both mocked and unmocked plugin in the same test, which sounds like a rare corner case, unless I'm missing some scenario here 🤔 Doesn't hurt to override config name here though, generally not that we should rely on it in any way externally, can do that!

Just needs to happen in the same process as long as the global app registry is truly global, right? That doesn’t seem all that unlikely, and it’s going to be hard to diagnose because it could involve some “spooky action at a distance” from a completely unrelated test that also happens to render the plugin.

Another approach, maybe even a better one, would be figuring out how to best avoid having a truly global globalAppRegistry in tests at all. Ideally, tests should probably use a distinct one each time, not share the same on across all tests in that process…?

Ah, I see what you mean. Then I think we can change the code there that passes globalAppRegistry value to the context to be _globalAppRegistry ?? new AppRegistry(): if you don't care about the reigstry or don't need to interact with it inside the actual test, this would be just a safe fallback, but if you want to emit events or something, then you'd want to pass your instance there. How does that sound?

Yeah, I think that makes for some nice isolation between tests, and it should be easy enough to work around if it we do want the actual global instance?

Totally, and it would be super explicit if you want to pass the global one, it's a singleton export from the app-registry package. Will update, thanks for the feedback!

Updated globalAppRegistry to be always scoped as discussed in 72c9c15. Also when doing that stumbled on something that I think is a bug: we did call plugin.deactivate in appRegistry.deactivate, but didn't de-register plugins, changed that too to always clean-up plugins from registry as at the moment we call deactivate there, we expect this whole thing to be disposeable basically

…es helper

chore(databases-collection): refactor create db / create coll to a si…

80545c9

…ngle create namespace plugin

gribnoysup commented Nov 10, 2023

View reviewed changes

addaleax reviewed Nov 10, 2023

View reviewed changes

chore(databases-collections): re-introduce old tests; add plugin serv…

20f2bf6

…ice mock helper; add new plugin tests

gribnoysup marked this pull request as ready for review November 13, 2023 11:12

gribnoysup commented Nov 13, 2023

View reviewed changes

addaleax reviewed Nov 13, 2023

View reviewed changes

gribnoysup added 2 commits November 13, 2023 15:35

chore(app-registry): use scoped globalAppRegistry in withMockedServic…

72c9c15

…es helper

chore(instance): off -> removeListener

6691588

addaleax approved these changes Nov 13, 2023

View reviewed changes

gribnoysup added 2 commits November 13, 2023 17:34

Merge branch 'main' into compass-7410-create-namespace

b03e453

chore(databases-collections): only reset relevant state on open

0b01783

gribnoysup merged commit cbb9d01 into main Nov 14, 2023
5 checks passed

gribnoysup deleted the compass-7410-create-namespace branch November 14, 2023 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(databases-collection): refactor create db / create coll to a single create namespace plugin COMPASS-7410 #5100

chore(databases-collection): refactor create db / create coll to a single create namespace plugin COMPASS-7410 #5100

gribnoysup commented Nov 10, 2023 •

edited

Loading

gribnoysup Nov 10, 2023

addaleax Nov 10, 2023

addaleax Nov 10, 2023

addaleax Nov 10, 2023

gribnoysup Nov 13, 2023 •

edited

Loading

gribnoysup Nov 13, 2023

gribnoysup Nov 13, 2023

gribnoysup commented Nov 13, 2023

gribnoysup Nov 13, 2023

addaleax Nov 13, 2023

gribnoysup Nov 13, 2023

addaleax Nov 13, 2023

gribnoysup Nov 13, 2023

addaleax Nov 13, 2023

gribnoysup Nov 13, 2023

gribnoysup Nov 13, 2023 •

edited

Loading

chore(databases-collection): refactor create db / create coll to a single create namespace plugin COMPASS-7410 #5100

chore(databases-collection): refactor create db / create coll to a single create namespace plugin COMPASS-7410 #5100

Conversation

gribnoysup commented Nov 10, 2023 • 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

gribnoysup Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribnoysup commented Nov 13, 2023

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

gribnoysup Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

gribnoysup commented Nov 10, 2023 •

edited

Loading

gribnoysup Nov 13, 2023 •

edited

Loading

gribnoysup Nov 13, 2023 •

edited

Loading