Zaid-Ajaj / Feliz

A fresh retake of the React API in Fable and a collection of high-quality components to build React applications in F#, optimized for happiness
https://zaid-ajaj.github.io/Feliz/
MIT License
544 stars 83 forks source link

Enable prop inheritance by using instance members #53

Closed cmeeren closed 4 years ago

cmeeren commented 5 years ago

As discussed in https://github.com/cmeeren/Feliz.MaterialUI/issues/20, it would be nice to be able to inherit props.

I think the required changes are as follows:

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.)

Zaid-Ajaj commented 5 years ago

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?

cmeeren commented 5 years ago

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.

Zaid-Ajaj commented 5 years ago

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# :smile:

cmeeren commented 5 years ago

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.)

cmeeren commented 5 years ago

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

cmeeren commented 5 years ago

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 4 years ago

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

cmeeren commented 4 years ago

I think it can be closed.

Zaid-Ajaj commented 4 years ago

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