feat(dashboards-ng): event bus for widget communication#1647
feat(dashboards-ng): event bus for widget communication#1647chintankavathia wants to merge 12 commits intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a well-designed SiEventBus service for inter-widget communication, which is a significant and valuable feature. The implementation thoughtfully supports both Angular and non-Angular contexts by leveraging shared state on the window object, making it suitable for microfrontend architectures. The documentation and tests are comprehensive. My review includes suggestions to improve type safety by adhering to the project's style guide.
e0ac73d to
ca9ea13
Compare
| * mutable for internal event state updates. | ||
| */ | ||
| private get sharedEventsState(): Record<string, unknown> { | ||
| const win = window as unknown as Record<symbol, unknown>; |
There was a problem hiding this comment.
Can we potentially inject the window which is better testable and type-safe? Further we could consider to use the indexdb as store which is persistemt.
There was a problem hiding this comment.
No we cannot use any angular specific here as we need this to be used by non angular runtimes as well.
indexDb wouldn't be good choice here as we need synchronous data while indexDb is async moreover we don't want to persist data on page reload.
| * eventBus.emit('languageChange', 'de'); | ||
| * ``` | ||
| */ | ||
| export const getEventBusInstance = < |
There was a problem hiding this comment.
I would prefer the provider approach instead of a window + singleton mechanism or is this not possible?
There was a problem hiding this comment.
provider approach cannot be used as this is meant for only non angular widgets where it shouldn't import anything from @angular package
| * // => {key: 'processed' | 'sent', value: boolean}[] | ||
| * ``` | ||
| */ | ||
| snapshot(): EventNameToData<ET | SiEventType>; |
There was a problem hiding this comment.
From a security / privacy perspective I wouldn't provide a full list of all states at once - consumers should know the events they want to subscribe.
| // NOTE: Do not export si-event-bus.ts from here. It must be exported from the main entry point. | ||
| // This entry point exists so that non-Angular widgets (plain JS/TS, React, Vue, etc.) can | ||
| // import `getEventBusInstance` without pulling in any Angular dependency. | ||
| export * from './si-event-bus.base'; |
There was a problem hiding this comment.
We shouldn't expose the base class instead we should use interfaces
|
|
||
| Here is the [demo](https://github.com/siemens/element/blob/main/projects/dashboards-demo/src/app/pages/fixed-widgets-dashboard/fixed-widgets-dashboard.component.ts) | ||
|
|
||
| ### Communicating between widgets |
There was a problem hiding this comment.
Could we also add a usage chapter to the docs?
There was a problem hiding this comment.
currently our docs directly points to the readme for usage of flexible dashboards.
ca9ea13 to
4b0687d
Compare
4b0687d to
98c4e40
Compare
Introduces
EventBusservice to emit and subscribe events with data between widgets.Also introduces
getEventBusInstanceutil with separate entry point@siemens/dashboards-ng/event-busto allow non angular widgets to utilize same service.Usage
Documentation.
Examples.
Dashboards Demo.
Playwright report.
Coverage Reports: