Before hook for onRun of Catalog entities #3911

chenhunghan · 2021-09-29T13:17:05Z

Add extension api Renderer.Catalog.catalogEntities.addOnRunHook

Can be used like

Renderer.Catalog.catalogEntities.addOnRunHook(catalogEntityUid, (entity) => {
   // return true => invoke onRun, false => don't invoke onRun;
   return true;
});

Add prettier to be able to use .toMatchInlineSnapshot()

src/common/catalog/catalog-entity.ts

src/renderer/components/+catalog/catalog-entity.store.tsx

package.json

src/common/catalog/catalog-entity.ts

src/main/catalog-sources/weblinks.ts

Nokel81 · 2021-09-29T13:26:42Z

src/common/catalog-entities/web-link.ts

+  onClickDetailIcon(context: CatalogEntityActionContext) {
+    catalogCategoryRegistry
+      .getCategoryForEntity<WebLinkCategory>(this)
+      ?.emit("onClickDetailIcon", this, context);
+  }


From talking to @msa0311 I was under the impression that we were wanting to block the "onRun" if any of these pre-hooks calls the equivalent of preventDefault which this form cannot support because the handlers are run concurrently.

src/renderer/components/+catalog/catalog-entity-details.tsx

jim-docker · 2021-09-29T15:09:03Z

I don't understand the need for this but instead of adding onClickDetailIcon can't you add something to the run context that lets onRun determine if the icon was clicked? Breaking change?

Nokel81 · 2021-09-29T15:13:33Z

I don't understand the need for this but instead of adding onClickDetailIcon can't you add something to the run context that lets onRun determine if the icon was clicked? Breaking change?

That would also be a possible solution. However, according to @msa0311 this is desirable to happen on attempts for onRun. So from that POV the name of even this proposed function is wrong.

chenhunghan · 2021-09-30T12:56:46Z

I don't understand the need for this but instead of adding onClickDetailIcon can't you add something to the run context that lets onRun determine if the icon was clicked? Breaking change?

Because we need to add the hooks to onRun via extension, from extension there is no way of changing onRun...

src/common/catalog-entities/general.ts

src/common/catalog-entities/kubernetes-cluster.ts

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

src/extensions/renderer-api/catalog.ts

Nokel81 · 2021-10-04T15:31:50Z

src/extensions/renderer-api/catalog.ts

+  /**
+   * Returns one catalog entity onRun hook by catalog entity uid
+   */
+  getOnRunHook(catalogEntityUid: CatalogEntity["metadata"]["uid"]): CatelogEntityOnRunHook | undefined {
+    return registry.getOnRunHook(catalogEntityUid);
+  }
+


We should support several onRunHooks. You sort of do already but not very well since you only ever return the first hook.

I think this should be another PR,

No because this will have to be supported until the next major

At least in current timeframe I don't think I can do this before 5.3. @Nokel81 would you like to to take over on the multiple hooks part?

src/renderer/api/catalog-entity-registry.ts

Nokel81 · 2021-10-04T15:34:08Z

src/renderer/api/catalog-entity-registry.ts

+  protected entityOnRunHooks = observable.set<{
+    catalogEntityUid: CatalogEntityUid;
+    onRunHook: CatelogEntityOnRunHook
+  }>([], {
+    deep: false,
+  });


This is a very strange type, why is it not observable.map<string, CatalogEnitytOnRunHook[]>([], { deep: false });?

We need catalogEntityUid because there are getOnRunHook

The top level map's key would be CatalogEntityUid

string is not as obvious as catalogEntityUid, but again, just my opinion, maybe you know more about the convention in Lens.

We don't really have a convention... (namely we use both styles). What I meant was I should have suggested the type observable.map<CatalogEntityUid, IObservableSet<CatelogEntityOnRunHook>>

OK, thanks for clarify.

Changed c863134

Nokel81 · 2021-10-04T15:35:25Z

src/renderer/components/+catalog/catalog-entity-details.tsx

-                onClick={() => item.onRun(catalogEntityRunContext)}
-                size={128} />
+                onClick={() => {
+                  this.props.onClickDetailPanelIcon(item);


I don't think we should pass this in as props, we should have an unexported function that takes an CatalogEntity and does the work once (DRY as it were).

Can the refactoring be another PR?

src/renderer/components/+catalog/catalog-entity-item.tsx

Nokel81 · 2021-10-04T15:37:53Z

src/renderer/components/+catalog/catalog-entity-item.tsx

+    if (!onRunHook) {
+      this.entity.onRun(ctx);
+
+      return;
+    }
+
+    if (typeof onRunHook === "function") {


These two checks can be combined:

if (typeof onRunHook !== "function") { ... return; } ...

Maybe, but with less readability IMHO.

Nokel81 · 2021-10-04T15:39:35Z

src/extensions/renderer-api/catalog.ts

+   * @param onRunHook The function that should return a boolean if the onRun of catalog entity should be triggered.
+   * @returns A function to remove that hook
+   */
+  addOnRunHook(catalogEntityUid: CatalogEntity["metadata"]["uid"], onRunHook: CatelogEntityOnRunHook) {


I think this should be instead called onBeforeRun so as to be more descriptive of when it will be run.

If we want to support multiple hooks in the future, onBeforeRun is less descriptive IMO.

How is it less descriptive? It better shows when we plan on running them

Well, it's just my opinion, if you think "onBeforeRun" is better, I would just change it

I meant the argument would be onBeforeRun and this method would be addOnBeforeRun

OK, addOnBeforeRun makes sense.

Edited 74eb89c

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

chenhunghan · 2021-10-05T06:56:45Z

src/renderer/components/icon/icon.tsx

@@ -114,14 +114,14 @@ export class Icon extends React.PureComponent<IconProps> {
    };

    // render as inline svg-icon
-    if (svg) {
+    if (typeof svg === "string") {


This is here just for easier mocking in tests (less crashes in components)

chenhunghan · 2021-10-05T06:57:41Z

src/extensions/lens-renderer-extension.ts

@@ -77,7 +77,7 @@ export class LensRendererExtension extends LensExtension {
  }

  /**
-   * Add a filtering function for the catalog catogries. This will be removed if the extension is disabled.
+   * Add a filtering function for the catalog categories. This will be removed if the extension is disabled.


Was a typo.

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Signed-off-by: Sebastian Malton <[email protected]>

jim-docker

reviewed @Nokel81 's commits, LGTM but my head is spinning. @Nokel81 should give the final approval...

Signed-off-by: Sebastian Malton <[email protected]>

src/extensions/renderer-api/catalog.ts

Signed-off-by: Sebastian Malton <[email protected]>

chenhunghan added enhancement New feature or request area/catalog labels Sep 29, 2021

chenhunghan requested a review from a team as a code owner September 29, 2021 13:17

chenhunghan requested review from jweak and jim-docker and removed request for a team September 29, 2021 13:17

chenhunghan commented Sep 29, 2021

View reviewed changes

src/common/catalog/catalog-entity.ts Outdated Show resolved Hide resolved

chenhunghan commented Sep 29, 2021

View reviewed changes

src/renderer/components/+catalog/catalog-entity.store.tsx Show resolved Hide resolved

Nokel81 requested changes Sep 29, 2021

View reviewed changes

Nokel81 reviewed Sep 29, 2021

View reviewed changes

src/renderer/components/+catalog/catalog-entity-details.tsx Outdated Show resolved Hide resolved

msa0311 added this to the 5.3.0 milestone Sep 30, 2021

msa0311 added the blocker label Sep 30, 2021

msa0311 linked an issue Sep 30, 2021 that may be closed by this pull request

Hook for onRun of Catalog Entities #3916

Closed

chenhunghan changed the title ~~Emit "onClickDetailIcon" when user clicks the icon in detail panel~~ Hook for onRun of Catalog entities Sep 30, 2021

chenhunghan force-pushed the feat/emit-on-click-detail-icon branch from bcf0480 to d6f5517 Compare September 30, 2021 11:19

chenhunghan closed this Sep 30, 2021

chenhunghan reopened this Sep 30, 2021

chenhunghan force-pushed the feat/emit-on-click-detail-icon branch from 9d37487 to 600ee9d Compare October 1, 2021 05:28

Nokel81 reviewed Oct 1, 2021

View reviewed changes

src/common/catalog-entities/general.ts Outdated Show resolved Hide resolved

src/common/catalog-entities/kubernetes-cluster.ts Outdated Show resolved Hide resolved

chenhunghan added 8 commits October 4, 2021 08:50

Add prettier to be able to use .toMatchInlineSnapshot

e2f5978

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Add 'onClickDetailIcon' event when user clicks the icon detail panel

b9c485d

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Add triggerOnRunAfterOnClickDetailIcon

aed5760

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Move to .spec

22787a1

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Fix test description

4ebbccb

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Fix typo

184612e

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Fix indents

7cb1219

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Fix indents

74ba7ed

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Test if onRun get trigger, and use inlineSnapshot

a877e10

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Nokel81 reviewed Oct 4, 2021

View reviewed changes

chenhunghan added 10 commits October 5, 2021 08:22

remove getAllOnRunHooks

613f84b

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Fix typo CatelogEntityOnRunHook > CatalogEntityOnRunHook

6fe1809

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Remove CatalogEntityItem

c053286

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Add defaultProps.catalogEntityStore

f0b8479

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Should be optional

bbde39e

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Update tests

fcdd032

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Add addOnRunHook return false test

d65e970

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Add tests when addOnRunHook return a promise

2ba9315

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Catch the cases where onRunHook throws or reject

24f5f99

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Samne to <HotBarMenu />

807aefd

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

chenhunghan commented Oct 5, 2021

View reviewed changes

chenhunghan requested a review from jakolehm October 5, 2021 07:01

chenhunghan and others added 4 commits October 5, 2021 16:11

onRunHook > onBeforeRun

74eb89c

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

observable.set > observable.map

c863134

Signed-off-by: Hung-Han (Henry) Chen <[email protected]>

Support multiple onBeforeHooks per entity

44a8824

Signed-off-by: Sebastian Malton <[email protected]>

Fix tests

c264d5f

Signed-off-by: Sebastian Malton <[email protected]>

jim-docker reviewed Oct 5, 2021

View reviewed changes

Nokel81 added 2 commits October 5, 2021 14:40

Cleanup and fix activate-entity

87babc8

Signed-off-by: Sebastian Malton <[email protected]>

Fix

fe212ad

Signed-off-by: Sebastian Malton <[email protected]>

jim-docker reviewed Oct 5, 2021

View reviewed changes

src/extensions/renderer-api/catalog.ts Outdated Show resolved Hide resolved

Fix docs

5c3337c

Signed-off-by: Sebastian Malton <[email protected]>

Nokel81 requested a review from jim-docker October 5, 2021 19:17

Nokel81 approved these changes Oct 5, 2021

View reviewed changes

Nokel81 changed the title ~~Hook for onRun of Catalog entities~~ Before hook for onRun of Catalog entities Oct 5, 2021

Nokel81 merged commit 4af796c into master Oct 5, 2021

Nokel81 deleted the feat/emit-on-click-detail-icon branch October 5, 2021 19:36

jim-docker mentioned this pull request Oct 6, 2021

release v5.3.0-alpha.0 #3966

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Before hook for onRun of Catalog entities #3911

Before hook for onRun of Catalog entities #3911

chenhunghan commented Sep 29, 2021 •

edited

Loading

Nokel81 Sep 29, 2021

jim-docker commented Sep 29, 2021

Nokel81 commented Sep 29, 2021

chenhunghan commented Sep 30, 2021

Nokel81 Oct 4, 2021

chenhunghan Oct 5, 2021

Nokel81 Oct 5, 2021

chenhunghan Oct 5, 2021

Nokel81 Oct 4, 2021

chenhunghan Oct 5, 2021

Nokel81 Oct 5, 2021

chenhunghan Oct 5, 2021

Nokel81 Oct 5, 2021

chenhunghan Oct 5, 2021

chenhunghan Oct 5, 2021

Nokel81 Oct 4, 2021

chenhunghan Oct 5, 2021

Nokel81 Oct 4, 2021

chenhunghan Oct 4, 2021

Nokel81 Oct 4, 2021

chenhunghan Oct 5, 2021

Nokel81 Oct 5, 2021

chenhunghan Oct 5, 2021

Nokel81 Oct 5, 2021

chenhunghan Oct 5, 2021

chenhunghan Oct 5, 2021

chenhunghan Oct 5, 2021

chenhunghan Oct 5, 2021

jim-docker left a comment

Before hook for onRun of Catalog entities #3911

Before hook for onRun of Catalog entities #3911

Conversation

chenhunghan commented Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

jim-docker commented Sep 29, 2021

Nokel81 commented Sep 29, 2021

chenhunghan commented Sep 30, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jim-docker left a comment

Choose a reason for hiding this comment

chenhunghan commented Sep 29, 2021 •

edited

Loading