Zaid-Ajaj/Feliz

Enable prop inheritance by using instance members

Closed this issue · 9 comments

As discussed in Shmew/Feliz.MaterialUI#20, it would be nice to be able to inherit props.

I think the required changes are as follows:

  • Change this to type htmlProps () (or another name)

    type prop =

  • Change all htmlProps static members to instance members

  • Add a value let prop = htmlProps ()

  • Remove this module definition

    module prop =

  • Optionally rename the "enum" prop types contained in that module, e.g. roleProp

    type role =

  • Make the enum prop types to normal classes, e.g. roleProp (), and change all static members to instance members

  • Move those enum prop types above the main prop type (htmlProps)

  • Add members to htmlProps corresponding to those types, set to an instance of the types the modules were converted to (remember to add the corresponding documentation). For example:

    /// https://www.w3.org/WAI/PF/aria-1.1/roles
    member _.role = roleProp ()

    (This may be optimized by instantiating them in the constructor instead of on every access.)

I have no idea whether any of this impacts bundle size.

Oh, and optionally also add a type PropNotSupported = private PropNotSupported that can be used by 3rd party libraries to shadow/disallow a property that should not be inherited. (It's no problem defining this in 3rd party libraries where needed, but it might also be nice to have a single, idiomatic type for it. I don't have a very strong opinion on this.)

Hello @cmeeren, thanks a lot for the detailed steps! Though because of this comment in Feliz.MaterialUI#20 it is a show stopper for me, really. Combining enums and functions in one value is soo nice to have and I am using it extensively both in Feliz and now in Feliz.Recharts, makes things so much easier and the code is clearer that other alternatives.

For Feliz.MaterialUI, because it really makes implementation easier, isn't it an idea to introduce a specific baseProps or htmlProps for just Feliz.MaterialUI and have the components there inherit that base type?

Just to be 100% clear: With the "alternative method name" syntax, this would be the usage:

popover.anchorOrigin.topLeft  // normal prop like today
popover.anchorOrigin'(10, 20)  // method like today but with different name

And the other "property as method" option:

popover.anchorOrigin().topLeft  // "prop" like today but as a method to access values
popover.anchorOrigin(10, 20)  // method like today

And both of those are deal-breakers for you, am I right?

For Feliz.MaterialUI, because it really makes implementation easier, isn't it an idea to introduce a specific baseProps or htmlProps for just Feliz.MaterialUI and have the components there inherit that base type?

I'm not sure which implementation you refer to. If you refer to the Feliz.MaterialUI implementation, then no, the implementation is definitely easier the way it is today, without inheritance or duplicated props (particularly duplicated props from Feliz). If you instead refer to the user's implementation (when using Feliz.MaterialUI), then I agree that it would be more pleasant to have a single place to look for props (e.g. only popover, and not paper or prop), though I'm far from convinced it matters enough to justify the added maintenance burden. Personally I have no problem having Mui.popover with some props from popover and other props from prop.

And both of those are deal-breakers for you, am I right?

Yes, not a big fan of the syntax of either really. Again very subjective of me I know, I need to think about it. Though it looks like you almost solved this one here, type resolution is really fascinating in F# 😄

Yep, will experiment with that. If it works (which I think it will), no changes are needed in Feliz.

(Though of course Feliz is free to split the props for separate components, as previously mentioned. It might be nice to have the same Html.someComp [ someComp.someProp ] syntax for the whole Feliz ecosystem, and it might declutter the prop API by removing unusable props.)

My attempted approach broke down for enum props. So we're back to start, with enum props being the primary hindrance for enabling inheritance.

Another option is not implementing enum props in modules, but as normal props, with underscore-separated names:

type appBar =
  static member position_fixed = // ...
  static member position_absolute = // ...

Usage:

Mui.appBar [
  appBar.foo 1
  appBar.bar "baz"
  appBar.position_fixed
]

This would enable "static inheritance" as described in the Feliz.MaterialUI issue. However, there's always the danger of name collision with real props, and it's not clear exactly how to document the prop vs. the enum values.

Shmew commented

Is there anything left to discuss on this or are we good to close it?

I think it can be closed.

Yeah this can closed. Thanks a lot @cmeeren for the suggestions, it was great to see what other options we had before moving forward with the current solution