Shmew / Feliz.MaterialUI

Feliz-style Fable bindings for Material-UI
https://shmew.github.io/Feliz.MaterialUI/
MIT License
70 stars 19 forks source link

Rewrite Object.fromFlatEntries in F# #58

Closed inosik closed 3 years ago

inosik commented 3 years ago

This works around a regression in Fable 3, see fable-compiler/Fable#2249.

Note that the way this helper is written now, it will be added to every call site where it's used, so it will increase the size of the compiled JS code a bit. Maybe it's better to put it into a JS file and use one of Fables import* functions to import it? Or rewrite it in F# altogether, as suggested here?

cmeeren commented 3 years ago

Thanks a lot!

Note that the way this helper is written now, it will be added to every call site where it's used, so it will increase the size of the compiled JS code a bit. Maybe it's better to put it into a JS file and use one of Fables import* functions to import it? Or rewrite it in F# altogether, as suggested here?

I'd like to understand this a bit better before merging. Why is it added to every call site? It's not inline, which is why I'm confused. Why would rewriting it in F# or importing from a JS file help?

alfonsogarciacaro commented 3 years ago

Thanks for this @inosik. As you says, it's probably preferred to just use F# or an external JS helper to avoid too much code duplication.

@cmeeren The way Emit works is by emitting the code string (replacing the argument placeholders) in each call site, it doesn't actually declare a function with the JS code. There's no particular reason for that, it's just that Emit was originally designed for simple use cases and it has always remained this way. I did try at one point to add an EmitDeclaration attribute but there were some issues with Babel so this was never documented.

inosik commented 3 years ago

Emit is used for emitting expressions, not to actually declare functions. This is why it gets inserted at every call site. So when using the helper function, it's as if we were writing something like this in F#:

let obj =
  let rec setProperty target key value = ...
  let target = ... // empty object
  for (key, value) in props do
    setProperty target key value
  target
cmeeren commented 3 years ago

Thanks for the explanations!

@inosik Are you able to update the PR to use the F# implementation? Also, as a quick test, are you able to run the dotnet fake build -t Docs:Run to check that the docs app actually runs and looks correct?

inosik commented 3 years ago

Done. As far as I can tell, the demo app looks right.

cmeeren commented 3 years ago

Thanks a lot! I'll merge and deploy.

cmeeren commented 3 years ago

1.2.2 is in the pipeline now. Thanks again!