feat(AppShell): add Service Manager package to the App Shell suite [PPUC-187] #4895
Conversation
loonatic
requested review from
zettca and
francisco-guilherme
and removed request for
a team
loonatic
force-pushed
the
PPUC-187
branch
2 times, most recently
from
3e0ad36 to
7b891d1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
loonatic
force-pushed
the
PPUC-187
branch
2 times, most recently
from
898efce to
6e3c5fc
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new Service Manager package (@hitachivantara/app-shell-services) to the App Shell suite, providing service registration, resolution, and React hooks for service consumption. The implementation includes support for multiple service provider types (instances, bundles, factories, and components) with configurable error handling strategies.
Key changes:
- Creates the complete
app-shell-servicespackage with TypeScript types, utilities, and React hooks - Integrates the Service Manager into the App Shell UI through a custom hook initializer
- Extends the App Shell configuration to support services configuration
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/app-shell-services/ | New package containing service management system with types, hooks, and utilities |
| packages/app-shell-ui/src/components/CustomHooksInitializer/CustomHooksInitializer.tsx | Adds service manager initialization hook |
| packages/app-shell-ui/package.json | Adds dependency on new app-shell-services package |
| packages/app-shell-shared/src/types/Config.ts | Extends configuration types to include services config |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| TComponent extends FC<PropsWithChildren>, | ||
| P extends Record<string, unknown>, | ||
| >(Component: TComponent, configProps: P): TComponent { | ||
| const BoundComponent = (props: PropsWithChildren) => { | ||
| // @ts-expect-error TODO fix the types! | ||
| return <Component {...props} {...configProps} />; | ||
| }; | ||
|
|
||
| return BoundComponent as TComponent; |
There was a problem hiding this comment.
The @ts-expect-error comment indicates unresolved type issues. Consider defining proper intersection types for component props or using React.ComponentProps to ensure type safety.
| TComponent extends FC<PropsWithChildren>, | |
| P extends Record<string, unknown>, | |
| >(Component: TComponent, configProps: P): TComponent { | |
| const BoundComponent = (props: PropsWithChildren) => { | |
| // @ts-expect-error TODO fix the types! | |
| return <Component {...props} {...configProps} />; | |
| }; | |
| return BoundComponent as TComponent; | |
| TComponent extends FC<any>, | |
| TProps extends object = React.ComponentProps<TComponent> | |
| >(Component: TComponent, configProps: Partial<TProps>): FC<Omit<TProps, keyof typeof configProps> & PropsWithChildren> { | |
| const BoundComponent: FC<Omit<TProps, keyof typeof configProps> & PropsWithChildren> = (props) => { | |
| return <Component {...configProps} {...props} />; | |
| }; | |
| return BoundComponent; |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This one is a complicated scenario, even the suggestion fails due to typings.
| const promiseFactoryRef = useRef(promiseFactory); | ||
| const pendingDataRef = useRef(pendingData); |
There was a problem hiding this comment.
The refs are not updated when promiseFactory or pendingData change, which means the effect will always use the initial values. These refs should be updated in the effect or the dependencies should include the actual parameters.
Copilot uses AI. Check for mistakes.
|
|
||
| Below is an example of how one could get all registered services (provided by multiple other packages/plugins in the AppShell ecosystem) for, for example, a header action dropdown menu. | ||
|
|
||
| First, and although not mandatory it would be ideal that the types of the services are defined in a separate package, so that both the service providers and consumers can depend on it: |
There was a problem hiding this comment.
I believe that it should be clearer, perhaps by explicitly stating it, what parts of the example pertain to the provider and what parts are of the consumer.
Also I couldn't find documentation regarding how services are registered by the provider. Which currently is apparently only possible through the appshell config.
| } | ||
|
|
||
| // Gets a reference's service as an async result. | ||
| export function useServiceReference<TService>( |
There was a problem hiding this comment.
The function names seem inconsistent. Some use useGet prefixes others use use prefix without the get. Is there some meaning to this pattern that I'm not understanding?
| const { services } = useHvAppShellConfig(); | ||
|
|
||
| return useMemo(() => { | ||
| serviceManagerInstance ??= createServiceManager(services); | ||
| return serviceManagerInstance; | ||
| }, [services]); |
There was a problem hiding this comment.
We are forcing services to be the ones being passed through appshell config. That is perfectly fine for our needs as it stands but I couldn't help thinking that where services come from could be decoupled from the generic logic of the service. Just food for thought..
No description provided.