hilla icon indicating copy to clipboard operation
hilla copied to clipboard

Allow defining custom FS route configuration

Open Legioth opened this issue 1 year ago • 4 comments
trafficstars

Describe your motivation

Custom menu building logic would benefit from a way of defining custom metadata in the route configuration. As an example, you might want to manually define a hierarchy or provide an additional row of text that should be shown separately from the view title.

Describe the solution you'd like

The initial FS router design suggested that unknown properties from export const config would just be passed through as-is so that they could be used in various parts of the system (not only for generating the menu). This does currently not seem to work as it was defined in the design.

After further consideration, I think it might be better to designate a specific property for this purpose since that makes the type definitions less unruly and removes concerns for backwards compatibility if we introduce new properties in ViewConfig. There would thus be a detail?: unknown property in ViewConfig and a corresponding property in MenuItem. createMenuItems could have a type parameter for asserting the detail type.

I could then do something like this in my view:

export const config: ViewConfig = { 
  title: 'Foo',
  detail: { description: 'Bar'} as MyViewDetails
 };

And render the menu like this:

{createMenuItems<MyViewDetails>().map(({ to, title, icon, detail}) => (
  <SideNavItem path={to} key={to}>
    {icon ? <Icon src={icon} slot="prefix"></Icon> : <></>}
    {title}<span>{detail.description}</span>
  </SideNavItem>
))}

Describe alternatives you've considered

The icon string can be abused to pass arbitrary data as long as you properly extract values out of the value when rendering the menu item.

Additional context

No response

Legioth avatar Jun 24 '24 08:06 Legioth

We can define a generic detail key on ViewConfig which also presents on the MenuItem object and is accessible from the createMenuItems. Under this key, the user would be able to define any arbitrary data they need for building a menu.

Lodin avatar Aug 06 '24 11:08 Lodin

Usage examples with combined suggestions from the team discussion:

export type MyViewDetails = Readonly<{
  description: string;
}>;

export const config: ViewConfig<MyViewDetails> = { 
  title: 'Foo',
  detail: { description: 'Bar'},
};

{createMenuItems<MyViewDetails>().map(({ to, title, icon, viewConfig}) => (
  <SideNavItem path={to} key={to}>
    {icon ? <Icon src={icon} slot="prefix"></Icon> : <></>}
    {title}<span>{viewConfig.detail.description}</span>
  </SideNavItem>
))}

platosha avatar Aug 06 '24 11:08 platosha

👍 this would solve my use-case.

Is it possible that the Developer doesn't have to specify the Type when using the createMenuItems function - can the type be inferred from the viewConfig that the function already holds? That way the MyViewDetails-Type could stay local.

Currently if someone changes ViewConfig<MyViewDetails> to something else e.g. ViewConfig<FoobarViewDetails> would createMenuItems have an typescript error?

Pascalmh avatar Aug 07 '24 06:08 Pascalmh

There's a bit of weak typing here that I don't see how to avoid. TypeScript doesn't have any way of knowing that createMenuItems has anything to do with export const config in some completely different file that isn't even directly imported in the file using createMenuItems. It's up to the diligence of the developer to keep those two in sync with each other and failing to do so will lead to runtime errors.

If you don't define any type parameter for the view config but just do export const config : ViewConfig = { ... }, then the type of the detail field will default to unknown which means that it's fully valid to assign anything to it. The example would thus not give any type error if written like this without a type parameter:

export const config: ViewConfig = { 
  title: 'Foo',
  detail: { description: 'Bar'},
};

On the receiving end, a plain createMenuItems() call will also default to using unknown as the type of the returned viewConfig.detail property. All the actual values will still be passed through but TypeScript will insist that you define what type you expect them to be before you can access them.

This would thus be a TypeScript error because unknown doesn't have a property named description:

createMenuItems().map(({viewConfig}) => console.log(viewConfig.detail.description));

This would compile and work as expected:

createMenuItems().map(({viewConfig}) => console.log((viewConfig.detail as MyViewDetails).description));

The createMenuItems<MyViewDetails> is basically just a shorthand for this approach.

It's the developer's responsibility to use the same type in export const config as they use in createMenuItems. There would be no TypeScript error but instead a runtime error if you'd do like this with the above export const config example:

type MyAltViewDetails = Readonly<{
  foo: string;
}>;
createMenuItems<MyAltViewDetails>().map(({viewConfig}) => console.log(viewConfig.detail.foo));

Alternatively, you could just go wild with any:

createMenuItems<any>().map(({viewConfig}) => console.log(viewConfig.detail.description));

Legioth avatar Aug 07 '24 06:08 Legioth