Comments (25)
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).
from feliz.
Hello @cmeeren, I guess we can consider this resolved, right? I will close the issue, please re-open for further discussion if needed
from feliz.
- Bundle size: if these functions aren't used (and there are 100s of them), they are still compiled anyway, inflating the bundle size AFAIK (the impact is of course still relative to total size of the application)
- Generic type arguments: irrelevant for Feliz but in context of Fable, a generic function with generic type arguments will not compile if the place of invocation is not inlined (all the generic type args have to be statically resolved at compile time) except for special cases where you can use
[<Inject>] ITypeResolver<'t>
as an optional argument of a static class (only highly specialized libraries use this feature, see Fable.SimpleJson/Thoth.Json)
from feliz.
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
from feliz.
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.
from feliz.
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 😄
from feliz.
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 😄)
from feliz.
This looks fantastic IMHO.
Thank you! Glad you like it 😄
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:
-
- Extend the
prop
static type with more functions
- Extend the
-
- Create a similar
prop
static type, specific to your library (maybeMui
) which has all the properties that the Mui requires. You could even aliasprop
withMui
and extendMui
with Mui-specific (overloaded) properties and functions.
- Create a similar
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
from feliz.
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
.
from feliz.
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
from feliz.
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?
from feliz.
I think babel does tree-shaking and removes unused functions when you make the production bundle. Inlining would defeat that.
from feliz.
@Luiz-Monad Are you then saying that, ideally, nothing in Feliz should be inlined? That inlining for bundle-size reasons is counter-productive?
from feliz.
@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 usedg()
contains a function -> not inline and used
You 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);
from feliz.
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
from 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 :)
from feliz.
@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
from feliz.
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?
from feliz.
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?
from feliz.
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.
from feliz.
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: dotnet/fsharp#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.
from feliz.
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
from feliz.
@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?
from feliz.
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. :)
from feliz.
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 😃
from feliz.
Related Issues (20)
- Q: Documentation on breaking changes in 2.6? [WAS Updating Feliz.UseListener to Feliz 2.6.0] HOT 4
- CSS overflow-anchor property HOT 1
- Error with Feliz 2.7 HOT 2
- Making UseElmish's dispatch function stable
- Feliz.Markdown escapeHtml doesn't seem to work HOT 1
- UseElmish: Failed to resolve import "use-sync-external-store/shim" HOT 3
- `react-markdown` removes the `escapeHtml` property HOT 1
- Clarification and Potential Improvement on PR #480 useEffectOnce behavior HOT 5
- Expanding Feliz.Rechart HOT 3
- Component created with a forwardRef that has generic type parameters loses state HOT 2
- "Directory import use-sync-external-store\shim is not supported" in UseElmish.fs.js HOT 2
- Use with Elmish documentation is lacking integration HOT 2
- Enhance Handling of F# Record Types as Props in ReactComponent Attribute HOT 4
- Explore if Feliz.CompilerPlugin can support POJO made using `[<ParamObject>]`
- prop.pattern produces invalid html HOT 4
- React 19 HOT 4
- Svg attribute `fill-rule` missing
- Update tests and move to github actions [Contributions Welcome]
- Doc changes havent been pushed to site HOT 1
- Feliz.popover auto positioning does not work
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from feliz.