brettkolodny / react-gleam

React bindings for Gleam
59 stars 2 forks source link

Rewrite the primitives (Work in progress) #20

Closed marcinkoziej closed 1 year ago

marcinkoziej commented 1 year ago

Hi @brettkolodny !

In context of the unable to maintain state issue and because I am live-testing react_gleam in my personal react-native project, I am trying to make sense of how this package could work in a way that benefits of correctness (coming from types and program structure) and better DX (less verbose, versatile, but concise).

A few observations/thoughts:

  1. React's functional components are (props) => Element functions. When using JSX, we employ the JSX mechanics that takes tag + its attributes, converts them into a JS object, and then calls this function. In current version of react_gleam, we also do this: we convert our Gleam types to Dynamic, so they work with attribute.Property, and then in a function component, we receive Dynamic props that we have to decode again. However, the JSX is converted to React.createElement(function, props, childrem), where props is an object. So why not just pass a Gleam type, without converting it both ways?.
  2. Same goes for externally loaded components (installed from npm) - we could just declare them as eg. pub external fn muiButton(props: MuiButtonProps) -> Element = "@material-ui/core" "Button", and create MuiButtonProps type that matches the props used by library, and use them as follows: element.create(muiButton, MuiButtonProps(primary: True, color: "#231234"), [element.text("Click me")])
  3. There is a slight problem with optionality of props fields. In gleam components, we can just make sure we always pass all the props as complete type, for external components (which could have 10+ props), we could either pass a type with Option(a) types, which gets converted to a or undefined, or we declare the props to contain a subset of imported props (just ones we use). The last option doesn't make sense for distributing such bindings though. Or perhaps such components should accept a Dynamic and that's it.
  4. We would still use element.node() kind of component creation for html tags (where the component is no a function but string), and where we pass List(Attribute) - it would be hard to create a type for all possible html tags...
  5. The element.node could also be used for externally loaded components, if declaring a prop type in advance (point 2) is not good DX. We could have element.native_node(fn () -> Element, List(Attributes), List(Element)) or similar, where 1st argument is function, not a string. We could even make it a default and have a function for each tag, eg: pub fn div () -> Element = "ffi.js" "div" and then export const div = (props) => (<div {...props}>{...props.children}</div>);...
  6. As for hooks, it can be really tricky to not fail to break their rules. I run into a problem where React runtime reported i have changed the order of calling them, and I could not figure out where – I figured out that hooks can be called recursively instead one after another, so we can do:
    pub fn counter (_props) => {
      use count, set_count <- state(0)
      use <- effect0(show_tada_emojis)
      ...
    }

    and this way you can't really skip a call to a hook, because the callback structure orders it..

  7. I do not know where to put everywhere used types such as Element or Context. I've put them in main react_gleam.gleam file to avoid import loops; however, Attribute is still in attribute.gleam because it's not used much elsewhere. Consistency would be useful here, but I am not sure how to achieve it - it also makes sense to put types in relevant domain-submodules.

Check out this PR for proposed changes, and here is a changed example from the issue - which works with these new primitives:

brettkolodny commented 1 year ago

Some really great work in this! I was hoping we'd figure out a way to avoid having to wrap components in a createElement call (hence what I was hoping use <- component would do) but I also came to the conclusion that it wasn't avoidable. On the bright side being able to do use count, set_count <- state(0) is great! (Though I'm still inclined to think it should stay taking a function. Maybe that could be a separate hook like state_fn?)

I think this is definitely a good path forward though. There has been talk about default arguments in Gleam which may help in the future but wont' be coming any time soon.

marcinkoziej commented 1 year ago

@brettkolodny I have added a state_lazy that accepts a function