Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

frontend: Add a way for plugins to react to Headlamp events #1455

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

joaquimrocha
Copy link
Collaborator

@joaquimrocha joaquimrocha commented Oct 13, 2023

Plugins may be interested in reacting to when e.g. all plugins are
loaded, in order to show a notification or perform some different
action. These changes allows plugins to do that by registering
callbacks that are run when some events in the app happen.

fixes #1451

@joaquimrocha joaquimrocha changed the title frontend: Add a way for plugins to react to Headlamp events WIP: frontend: Add a way for plugins to react to Headlamp events Oct 13, 2023
@ashu8912 ashu8912 force-pushed the headlamp-events branch 2 times, most recently from 8ab3689 to 0c92a0c Compare December 14, 2023 05:29
@ashu8912 ashu8912 marked this pull request as ready for review December 14, 2023 05:31
@ashu8912 ashu8912 force-pushed the headlamp-events branch 3 times, most recently from 6989416 to 5a26357 Compare December 14, 2023 08:41
@joaquimrocha joaquimrocha changed the title WIP: frontend: Add a way for plugins to react to Headlamp events frontend: Add a way for plugins to react to Headlamp events Jan 3, 2024
@joaquimrocha joaquimrocha force-pushed the headlamp-events branch 2 times, most recently from 78f6ab8 to b2cc779 Compare January 3, 2024 16:54
@joaquimrocha joaquimrocha requested a review from illume January 3, 2024 18:02
@illume
Copy link
Collaborator

illume commented Jan 5, 2024

@ashu8912 walked me through the PR.

Nice.

  • Should plugins be able to dispatch events too? Has this been considered. Is the dispatchEvent API exposed?
  • Probably needs an example plugin for documentation and testing reasons.
  • He queried the resource list event. I was also wondering about the name. Should it match the component name?

Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of notes for your consideration.

frontend/src/components/common/ObjectEventList.tsx Outdated Show resolved Hide resolved
frontend/src/lib/util.ts Outdated Show resolved Hide resolved
@illume
Copy link
Collaborator

illume commented Jan 15, 2024

Hi. There were some notes and questions that were missed because they were obscured with the other comment below. I'll repeat them here.

  • Should plugins be able to dispatch events too? Has this been considered. Is the dispatchEvent API exposed?
  • Probably needs an example plugin for documentation and testing reasons.
  • Ashu queried the resource list event. I was also wondering about the name. Should it match the component name?

I have some other feedback, but I'm not sure of the entire context or requirements. So I'm not clear if all of this applies.

  • Is there an issue for this somewhere? It would be good to link it to get more context and to see if it solves the problems there.

  • I like that there is only one exposed API for registration.

  • The API doesn't use the type checker for the data given to the callback. It would be better for it to have types (see below for an option)

  • It would be good to consider the number of files that need to be edited for this to add a new one. But I think it's pretty good. Maybe a little comment block inside of eventCallbackSlice explaining how to add one would be helpful.

On redux...

  • I like how it's possible to see these events in the store now. It will help with observability/debugging too inside the dev tools.
  • How does this compare to the plugins using the redux state? It seems a hook (typedselector) won't work for these types of events.
  • The events won't happen at the callsite, but will go through the redux store. I'm not sure if this matter for the requirements. But the implication of this is that folks won't be able to run code within the context of where the event is sent. Maybe this is a good thing. But I would like to know if you considered this.

names

  • Probably the dispatchHeadlampEvent could live inside the eventCallbackSlice rather than util.
  • I feel dispatchEvent is better because it's still a unique name within the headlamp codebase. On hover inside vscode it's easy enough to see where it's from, and also by looking at the import.

add types to data

interface ErrorBoundaryEvent {
  type: HeadlampEventType.ERROR_BOUNDARY;
  data?: { error: string };
}

interface DeleteResourceEvent {
  type: HeadlampEventType.DELETE_RESOURCE;
  data?: { resourceId: string };
}

type HeadlampEvent =
  | ErrorBoundaryEvent
  | DeleteResourceEvent
  | /* others */;

@joaquimrocha joaquimrocha force-pushed the headlamp-events branch 2 times, most recently from a12499c to d65373e Compare January 19, 2024 15:30
@joaquimrocha joaquimrocha requested a review from illume January 19, 2024 20:31
@joaquimrocha joaquimrocha marked this pull request as draft January 19, 2024 20:31
@joaquimrocha
Copy link
Collaborator Author

I updated the PR with a more complex, single commit, but I think it makes sense as it now makes things more structured, type checked, and easier to use. Also added an example plugin but that's still WIP as I haven't finished adding the details in the README, etc.

@joaquimrocha joaquimrocha marked this pull request as ready for review January 22, 2024 14:18
@joaquimrocha joaquimrocha force-pushed the headlamp-events branch 3 times, most recently from c419faf to eb685dd Compare January 30, 2024 17:21
@illume
Copy link
Collaborator

illume commented Feb 5, 2024

Some notes for future selves... I remember we spoke verbally, but I wanted to have it written down somewhere.

  • How would plugins who want to send events be able to do that? So plugin A can listen to an event from plugin B. There's no user requirement for this yet. However, I could see it possible to extend this to make an API for plugin authors in the future. Under it all it's just a { type, data }, and all the types in here are prefixed with "headlamp.". So a future plugin sending event API would just need to expose that.
  • Having functionality for different modules all in one file was a bit concerning. However, it's not such a great concern because there's not much cyclical importing of things going on. It only imports three internal things so far. But it does mean if there are 10x more things added that file will get quite crowded. I can roughly see a refactoring that might make this worthwhile to do when we get up to 10x more events, so I feel we do that then not now... if required. This current way seems more convenient, and better now.
  • For "The events won't happen at the callsite, but will go through the redux store. " there doesn't appear to be a use case for this. If there is later, I could see there being something like a 'sync' attribute added so the callbacks are called at the callsite.

Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very very Nice.

I left a few comments, now I'm testing it locally.

plugins/examples/headlamp-events/README.md Show resolved Hide resolved
frontend/src/redux/headlampEventSlice.ts Outdated Show resolved Hide resolved
plugins/examples/headlamp-events/package.json Outdated Show resolved Hide resolved
plugins/examples/headlamp-events/package.json Outdated Show resolved Hide resolved
plugins/examples/headlamp-events/README.md Show resolved Hide resolved
Plugins may be interested in reacting to when e.g. all plugins are
loaded, in order to show a notification or perform some different
action. These changes allows plugins to do that by registering
callbacks that are run when some events in the app happen.

Co-Authored-By: Ashu Ghildiyal <[email protected]>

Signed-off-by: Joaquim Rocha <[email protected]>
@joaquimrocha joaquimrocha marked this pull request as draft February 5, 2024 18:23
@joaquimrocha joaquimrocha marked this pull request as ready for review February 5, 2024 22:14
@joaquimrocha
Copy link
Collaborator Author

Thanks for the review @illume . I have updated the branch after your suggestions.

@joaquimrocha joaquimrocha requested a review from illume February 6, 2024 08:53
Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! Thanks.

@joaquimrocha joaquimrocha merged commit 253e709 into main Feb 7, 2024
9 checks passed
@joaquimrocha joaquimrocha deleted the headlamp-events branch February 7, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Headlamp event callbacks
2 participants