Making IManagedClasses<T>.managedClasses optional

[prabashn]

What is your feature request? Please describe.
Whenever I have a child component from FAST, I like to use the prop contract types FAST defines. For example for a text field, I would use TextFieldProps, which look like this:

export declare type TextFieldProps = ITextFieldHandledProps & ITextFieldUnhandledProps & ITextFieldManagedClasses;

Notice this type is composed of 3 types:

1. ITextFieldHandledProps - all partial props
2. ITextFieldUnhandledProps - all partial props
3. ITextFieldManagedClasses - not a partial. managedClasses is a required property.

Describe the solution you'd like
Making IManagedClasses.managedClasses optional so that when I need to create the prop object for a component, I don't need to keep passing in managedClasses: undefined (this is really problematic in unit tests for example).

Describe alternatives you've considered
As a work-around I'm passing 'managedClasses: undefined' everywhere this crops up (usually in unit tests), but would love to clean up the code by removing that unnecessary part.

Additional context
N/A

[nicholasrice]

Thanks for filing @prabashn - I noticed yesterday also, we need to omit the managedClasses prop from the styled components.

[chrisdholt]

@nicholasrice - I think this is regarding the base component. In the base component, this behavior is by design.

[nicholasrice]

I can see both sides here - at one level these are simply class-names and I shouldn't have to provide them if I don't want to.

There are a few ways this could play out - the prop itself could be optional, but if set, would require all keys. We could also make each key optional, meaning you could selectively apply class names or none at all.

I worry a bit about type safety in doing this - I like when the compiler tells us we're breaking something and I think we would lose some of that safety if we made these optional on the base.

[prabashn]

I maybe a little confused here. If managedClasses contain class1 and class2, and if one instance only class1 is specified, but in another only class2 is specified, yet in another both are specified (or not), why does it really matter? If it's there, the class will be applied to the element, otherwise it will not. Is there a reason why this needs to be enforced strictly? Is there some safety you gain by making it required (either the entire managedClasses prop itself, or the sub-properties that represent the CSS class names underneath managedClasses?) Is it to prevent devs from accidentally not specifying styles? But is there such a case that gains us anything? If the dev doesn't specify the class, the component will either not render as it should be, or it will render with the base styles that come with the control.

Sorry, just trying to understand the meaning of managedClasses, how it being required/optional changes meaning for component creators.

[nicholasrice]

Yep - I think I'm on board. I think the original goal was to prevent stylesheets from falling out of sync with class contracts, but I think that goal is actually moot. To your point, a dev shouldn't be required to use a class-name if it isn't necessary for their implementation.

[prabashn]

Currently I see us using IXXXHandledProps & IXXXUnhandledProps in a lot of places and one great thing by making it optional is you can stop doing this and just use IXXXProps. It's a lot easier to read, easier to type, and lot more meaningful (mostly because it's simply saying this is the full button contract). By making managedClasses optional, it makes this type a lot easier to use :)

[chrisdholt]

Some of ^^^^ is just implementation though. For instance, I think in a manageJss component instance, XXXXProps will work and not throw an error even if managedClasses is not passed as a prop. It's in base when you want to exclude that it creates issues, right?

What you're advocating for above makes it seem like the change is not within the className contract, but not requiring managedClasses in a union type. Is that accurate? That seems different than the original ask.

[prabashn]

I don't think that's right. manageJss expects a component that implements the full IManagedClasses. i.e.,

declare function manageJss<S, C>(styles?: ComponentStyles<S, C>): <T>(Component: React.ComponentType<T & IManagedClasses<S>>) => React.ComponentType<T & IJSSManagerProps<S, C>>;

Note this part:

React.ComponentType<T & IManagedClasses<S>>

So in order to call manageJss, the component that the JSS is being applied must contain the managedClasses, or else the compilation will fail. This basically means the prop contract for the component must contain managedClasses. And when this is a required property within the prop contract, it invades into all the code where you're trying to create props for the component.

[chrisdholt]

I should've been more clear, sorry to confuse. I confused myself while typing it and again now reading it.

Base component = <Component managedClasses={requiredHere} />

Styled component = <Component /> // managedClasses exists here so no error

The above aside though - as I don't think it matters much to the actual asks...

I think we have two asks on this:

1. In IManagedClasses, don't require all classes/contracts
2. In Union Type, don't include/require managed classes.

Can you confirm?