Closed cmeeren closed 4 years ago
This looks fantastic IMHO.
Thank you! Glad you like it :smile:
Is this simple to extend for 3rd party stuff, such as Material-UI?
Sure thing, but the approach is bit different because we are not using discriminated unions. I try to write something up when time permits but the idea is that you essentially have two options:
prop
static type with more functionsprop
static type, specific to your library (maybe Mui
) which has all the properties that the Mui requires. You could even alias prop
with Mui
and extend Mui
with Mui-specific (overloaded) properties and functions. Extending is as simple as:
type prop with
static member hello (value: string) = Interop.mkAttr "hello" value
Whether this is the "final form" of the library and the way to extend it, is still under consideration as I first want to try it out by building 3rd party libraries and see how it goes
Thanks! I'm already well underway with testing something for MUI, just for a fraction of the API, to see how it works. Will probably share sometime during the week/-end, it would be great if you could have a look at it just to see if I'm on track and following the Feliz spirit. Seems like success so far!
However, I haven't extended the existing prop
type. I have defined a separate prop
type in another namespace (Feliz.MaterialUI
). Works seemingly great; you of course get access to all members from all matching types if you open both Feliz
and Feliz.MaterialUI
.
I have a type Mui
that corresponds to Html
and contains the actual components.
(Currently I have placed component-specific props in separate submodules of prop
, as mentioned in #13.)
A possible point of improvement in Feliz is to have a reactElement
and createElement
that accepts not string
, but ReactElementType
(I think). so that we can call createElement (importDefault "@material-ui/core/Button")
. I have created these two helpers myself currently.
By the way, should all member be inline
? What are the pros/cons? I notice you didn't use inline
above, but everything in Feliz is inline
.
Thanks! I'm already well underway with testing something for MUI, just for a fraction of the API, to see how it works. Will probably share sometime during the week/-end, it would be great if you could have a look at it just to see if I'm on track and following the Feliz spirit. Seems like success so far!
That's awesome! I would definitely be happy to take a look if you
However, I haven't extended the existing prop type. I have defined a separate prop type in another namespace (Feliz.MaterialUI). Works seemingly great; you of course get access to all members from all matching types if you open both Feliz and Feliz.MaterialUI.
I have a type Mui that corresponds to Html and contains the actual components.
That's what I would do for Mui
(Currently I have placed component-specific props in separate submodules of prop, as mentioned in #13.)
It makes sense for Mui because of how many options you have
A possible point of improvement in Feliz is to have a reactElement and createElement that accepts not string, but ReactElementType (I think). so that we can call createElement (importDefault "@material-ui/core/Button"). I have created these two helpers myself currently.
I am reviewing the members I am using in the Interop
module, I just exposed whatever I was using in the library, will be reconsidered for the stable release
By the way, should all member be inline? What are the pros/cons? I notice you didn't use inline above, but everything in Feliz is inline.
I should have inlined the extended member above!
The rule of thumb is: if the value of the property is primitive like a string/int/bool/enum then inline the property but if your property computes a value based on the input then it is better not to inline because every time the user calls the inlined function, the whole function body is inlined in that place of invocation, so if a user uses the same function 10 times throughout the application, the function body is compiled inline 10 times instead of being defined once and referenced 10 times
The rule of thumb is: if the value of the property is primitive like a string/int/bool/enum then inline the property but if your property computes a value based on the input then it is better not to inline because every time the user calls the inlined function, the whole function body is inlined in that place of invocation, so if a user uses the same function 10 times throughout the application, the function body is compiled inline 10 times instead of being defined once and referenced 10 times
Nice to know! But (in the context of Fable) why inline in the first place, then? What's the benefit for the "simple" function/method bodies?
[<Inject>] ITypeResolver<'t>
as an optional argument of a static class (only highly specialized libraries use this feature, see Fable.SimpleJson/Thoth.Json)I think babel does tree-shaking and removes unused functions when you make the production bundle. Inlining would defeat that.
@Luiz-Monad Are you then saying that, ideally, nothing in Feliz should be inlined? That inlining for bundle-size reasons is counter-productive?
@Luiz-Monad What you are saying would be awesome! At least if compilation worked that way. Here is an example you can try with the REPL:
module App
type prop =
// does useless stuff
static member f() =
[ 1 .. 100 ]
|> List.map (fun x -> x * 20)
|> List.collect (fun n -> [n; n])
|> List.fold (+) 0
// does useless stuff
static member inline k() =
[ 1 .. 100 ]
|> List.map (fun x -> x * 20)
|> List.collect (fun n -> [n; n])
|> List.fold (+) 0
static member g() = 1
let value = prop.g()
printfn "%d" value
Where prop
contains:
f()
contains a function body -> not inlined and also not usedk()
contains a function body -> inlined but not used g()
contains a function -> not inline and usedYou would think that both f()
and g()
won't be compiled but that is not the case, f()
(not inlined and not used) is compiled anyway, but k()
(inlined, not used) doesn't make it into the compiled bundle
import { fold, collect, map, ofSeq, ofArray } from "fable-library/List.js";
import { type } from "fable-library/Reflection.js";
import { rangeNumber } from "fable-library/Seq.js";
import { toConsole, printf } from "fable-library/String.js";
import { declare } from "fable-library/Types.js";
export const prop = declare(function App_prop() {});
export function prop$reflection() {
return type("App.prop");
}
export function prop$$$f() {
return fold(function folder(x$$1, y) {
return x$$1 + y;
}, 0, collect(function mapping$$1(n) {
return ofArray([n, n]);
}, map(function mapping(x) {
return x * 20;
}, ofSeq(rangeNumber(1, 1, 100)))));
}
export function prop$$$g() {
return 1;
}
export const value = prop$$$g();
toConsole(printf("%d"))(value);
It does indeed work, but you need to configure webpack
for doing that, because what does the tree-shaking is not fable itself, so it won't work in the REPL.
Before
/// Library.fs
module Library
type prop =
// does useless stuff
static member f() =
[ 1 .. 100 ]
|> List.map (fun x -> x * 20)
|> List.collect (fun n -> [n; n])
|> List.fold (+) 0
// does useless stuff
static member inline k() =
[ 1 .. 100 ]
|> List.map (fun x -> x * 20)
|> List.collect (fun n -> [n; n])
|> List.fold (+) 0
type AppMain =
static member g() = 1
//// App.fs
module App
let value = Library.AppMain.g ()
printfn "%d" value
After
declare(function Library_prop() {
// see its empty, this weren't removed too because of `keep_classnames: true, keep_fnames: true ` in the terser plugin
});
declare(function Library_AppMain() {
});
!function toConsole(arg) {
return arg.cont(function (x) {
console.log(x)
})
}(printf('%d')) (1),
__webpack_require__.d(__webpack_exports__, 'value', function () {
return 1
})
Also, there's a caveat to your test, the entry file is kind of special in that functions from it aren't removed. ( I guess there's something to do with initialization of static classes, something has to call static initializer constructor to do side-effects on the modules )
See this repository I made for this testing https://github.com/Luiz-Monad/test-tree-shaking
Thanks a lot for the example repo! I will definitely investigate this further to see if inlining is actually doing anything useful in the context of Feliz
I will definitely investigate this further to see if inlining is actually doing anything useful in the context of Feliz
Cool, looking forward to hearing what you find :)
That's awesome! I would definitely be happy to take a look if you
Please check out the feliz
branch on cmeeren/fable-elmish-electron-material-ui-demo.
Most of the code is auto-generated based on the (HTML) API docs. There's a generator project (ugly and hacky, but gets the job done) and a project for the actual bindings. In the renderer project, only App.fs
has been converted to use the new Feliz-style bindings.
Please have a look at it when you feel like it, and let me know what you think and if you have any questions.
@cmeeren Looks very nice and much better than the current API IMHO but is it still a bit hard to read but that is more the nature of the library itself, it has a lot of very specific parts that you have to familiar with. I think there are parts that could be improved, take this example:
Mui.appBar [
prop.className c.appBar
prop.appBar.position.fixed'
prop.children [
Mui.toolbar [
prop.children [
Mui.typography [
prop.typography.variant.h6
prop.typography.color.inherit'
prop.text (pageTitle model.Page)
]
]
]
]
]
My personal perfect version of this snippet would be to turn it into the following:
Mui.appBar [
AppBar.className c.appBar
AppBar.position.fixed'
AppBar.children [
Mui.toolbar [
Toolbar.children [
Mui.typography [
Typography.variant.h6
Typography.color.inherit'
Typygraphy.text (pageTitle model.Page)
]
]
]
]
]
This way it is easy to find Mui elements because you can just "dot through" Mui and once you found your element (appBar
), you can "dot through" the module name (AppBar
) to define the properties and such
Maybe keep
AppBar
as lowercase as well
I think you get the idea but to be more exact, the general syntax of this API is as follows where {Element}
is a react element:
Mui.{element} [
{Element}.{propName}.{propValue}
{Element}.children [
Mui.{otherElem} [
{OtherElem}.{propName}.{propValue}
// etc.
]
]
]
What do you think about this? This API also inspired the library the ideas of fabulous-simple-elements if you want to see a concrete implementation
I think that's perfect, and just the kind of thing I wanted to get your opinion on. I initially chose to have everything under prop
since that's how the main library worked, but of course you don't really have any component-specific props, whereas MUI has more more less only component-specific props.
I think sticking to lowercase module names may look best (and save an extra keystroke), but I'm open to counter-arguments.
Good that I have auto-generated this stuff, that makes it simple to change.
I'll update and let you know.
There's one thing in particular I'd like to get opinions on, though. It's the ClassName
stuff. The makeStyles
hook returns an object with the same props as the one it's passed in, but where every (JSS) element has been replaced by a string, which is the class name to use (and can be passed to e.g. prop.className
).
Now, there's no way to type that in F#, so I have to work with what I have. Normally all props of the style object are IStyleAttribute list
. This means that I can add an overload for prop.className
which accepts IStyleAttribute list
, which of course is a lie since at runtime it's a string. If you actually passed it IStyleAttribute list
it would fail. In addition to prop.className
, this also goes for all the classes.<element>.<classesName>
(used in <element>.classes [ ... ]
). They also accept a class name (string).
What I have done, as you can see, is to "require" all the IStyleAttribute list
properties of the style object to be wrapped in asClassName
, which basically just unboxes to IClassName
(a proxy for string
, if you like). And then I have added an overload to prop.className
accepting IClassName
, and made all the classes
props accept IClassName
. I like that it's more strongly typed, but I don't like that it requires extra typing (asClassName
for every top-level CSS rule). The compiler will complain if you miss it, but it won't tell you what to do, and it's still extra noise.
Do you have any input on this?
Also, I noticed this:
listItem.divider ((page = Home))
Here, double parenetheses are required, since otherwise F# interprets it as trying to call listItem.divider
with the (non-existent) page
parameter set to Home
(instead of the value
parameter set to page = Home
). Do you see a way to avoid that?
Hello @cmeeren, first of all, I freaking love this syntax:
Mui.appBar [
prop.className c.appBar
appBar.position.fixed'
appBar.children [
Mui.toolbar [
toolbar.children [
Mui.typography [
typography.variant.h6
typography.color.inherit'
prop.text (pageTitle model.Page)
]
]
]
]
]
It just looks so clean and so simple! Though if I were you, I would have probably duplicated some of the generic prop
functions into component-specific props such as appBar.className
instead of (or along side) prop.className
so that they all look symmetrical but more importantly to give the IClassName
overload to the Mui-specific component instead of the generic prop.className
that takes a string because makeStyles
is also a Mui-specific construct and it makes sense that it will only apply to Mui components.
I think you tackled the makeStyles
the best way possible, at least right now I can't think of a better way (Although I am not big fan of asClassName
, maybe Styles.createClass
instead? it's up to you).
As for listItem.divider ((page = Home))
it is tricky, you could add a dummy function let when (x: bool) = x
but that is just noise. I think it is best to file it as a compiler bug because I can't think of reason why F# compiler can't resolve the proper overload of the function, I haven't tried myself but I will look into it when time permits
Lastly, I am not as responsive as usual this week cause I am on vacation now, so I might not be able to read/check everything etc.
Though if I were you, I would have probably duplicated some of the generic
prop
functions into component-specific props such asappBar.className
instead of (or along side)prop.className
so that they all look symmetrical but more importantly to give theIClassName
overload to the Mui-specific component instead of the genericprop.className
that takes a string becausemakeStyles
is also a Mui-specific construct and it makes sense that it will only apply to Mui components.
Check it out now :) I have made drastic improvements and expansions in the last days, just pushed them (not done, I'm currently going through all MUI components to verify and improve the generated props).
I think you tackled the
makeStyles
the best way possible, at least right now I can't think of a better way (Although I am not big fan ofasClassName
, maybeStyles.createClass
instead? it's up to you).
I'd like to keep it as short as possible since it'll be used a lot, but I'm open for other names. Though I have half a mind to just have an IStyleAttribute list
overload. It'll remove potentially quite a bit of noise, and I doubt it'll be very dangerous even if it can technically be used incorrectly.
As for
listItem.divider ((page = Home))
it is tricky, you could add a dummy functionlet when (x: bool) = x
but that is just noise. I think it is best to file it as a compiler bug because I can't think of reason why F# compiler can't resolve the proper overload of the function, I haven't tried myself but I will look into it when time permits
Thanks, I've filed an issue now: https://github.com/dotnet/fsharp/issues/7423
Lastly, I am not as responsive as usual this week cause I am on vacation now, so I might not be able to read/check everything etc.
Gotcha, no problem. I'll keep posting issues if I come across things, and you just reply in your own time.
Check it out now :) I have made drastic improvements and expansions in the last days, just pushed them (not done, I'm currently going through all MUI components to verify and improve the generated props).
Looks really good, with the generated docs too 😍 maybe time to put in it's own repo?
I'd like to keep it as short as possible since it'll be used a lot, but I'm open for other names. Though I have half a mind to just have an IStyleAttribute list overload. It'll remove potentially quite a bit of noise, and I doubt it'll be very dangerous even if it can technically be used incorrectly.
That would work too, Fable libraries cheat the type-system all the time ;)
Thanks, I've filed an issue now: dotnet/fsharp#7423
Awesome! thanks a lot
Looks really good, with the generated docs too 😍 maybe time to put in it's own repo?
I've been thinking about that, but there are still a few important bugs (e.g. #27) I'd rather figure out with the convenience of having everything in the same place, so I think I'll keep it there until it's ready for a prerelease on nuget (hopefully won't be too long).
@Zaid-Ajaj I'm just about done with Feliz.MaterialUI. Will publish to a separate repo soon. It would be great to have you review it, first and foremost to check some design decisions and ensure consistency with Feliz, but also to check some implementation stuff (e.g., am I using things you'll be making internal, or are there things I'm not using from Feliz but should be using).
When I create the new repo, is it OK if I create issues explaining what I'd like reviewed, and tag you?
I have now uploaded a draft of Feliz.MaterialUI to cmeeren/Feliz.MaterialUI. I have created several issues with things I'd like reviewed.
I would be very grateful if you could find the time to have a look at them!
There is no need to spend a lot of time on each issue; I really just want a second opinion for the reasons mentioned in my previous comment. I'm happy to go into the weeds if you want, but even just "this looks fine" would be great.
There's no rush, of course. :)
Awesome work @cmeeren! At first glance, the bindings look very clean, I will take a look at each issue in the next few days, I promise :smile:
Hey! Any chance to continue looking at the issues? Again no hurry, just a friendly reminder since I haven't heard from you for a couple of weeks 😃
I have actually looked at the issues but like I said before, whether an API is good or not comes from use cases, that is why I asked you to publish an alpha version so that I could try it out but I haven't had the time do so yet (it has been on the back of my mind this week :smile:)
Hello @cmeeren, I guess we can consider this resolved, right? I will close the issue, please re-open for further discussion if needed
This looks fantastic IMHO.
Is this simple to extend for 3rd party stuff, such as Material-UI?
It would be great to have some kind of guide on how to do that. :)