Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up memo overloads #118

Closed
cmeeren opened this issue Nov 15, 2019 · 10 comments · Fixed by #186
Closed

Clean up memo overloads #118

cmeeren opened this issue Nov 15, 2019 · 10 comments · Fixed by #186
Labels
enhancement New feature or request Feliz Issues related to Feliz

Comments

@cmeeren
Copy link
Contributor

cmeeren commented Nov 15, 2019

React.memo has tons of overloads, seemingly for any combination of parameters. Many overloads makes it hard to find the one I need. I suggest having fewer overloads (one?) with optional parameters instead.

@Zaid-Ajaj
Copy link
Owner

I will re-evaluate and review all React.* functionality when I start documenting them with examples. Will be taking these kinds of decisions into account

@cmeeren
Copy link
Contributor Author

cmeeren commented Nov 15, 2019

Thanks!

By the way (unrelated, sorry), would it be an idea to have functionComponent be able to memoize, just like Fable.React's FunctionComponent.Of? IMHO it's a nice convenience, but I might have missed a simple way to use Feliz's React.memo (given that it's undocumented).

@Shmew
Copy link
Collaborator

Shmew commented Nov 15, 2019

@zanaptak's PR has some discussion on this.

The main conclusion being:

Required parameter in last position means we can't use optional args anyway so I added the billion overloads.

@Shmew
Copy link
Collaborator

Shmew commented Nov 15, 2019

What if we had a couple normal overloads and then one overload that takes a feeder function like DefaultMemoOptions -> DefaultMemoOptions and they can set what they want there?

@cmeeren
Copy link
Contributor Author

cmeeren commented Nov 15, 2019

Could you give an example or two of the usage you have in mind?

@Shmew
Copy link
Collaborator

Shmew commented Nov 15, 2019

Yeah sure, so we'd have the normal overloads and then a single overload for options like this:

type DefaultMemo<'T> =
    { AreEqual: ('T -> 'T -> bool) option
      Name: string option
      WithKey: ('T -> string) option }

type React =
    static member inline memo (render: 'props -> ReactElement) = Internal.memo render
    static member inline memo (render: 'props -> ReactElement, options: DefaultMemo<'T> -> DefaultMemo<'T>) = Internal.memoWithOpts render options

Which would then let you do things like this:

renderFun being some 'props -> ReactElement function

React.memo renderFun
React.memo (renderFun, fun d -> { d with Name = Some "SomeKey" })
React.memo (renderFun, fun d -> { d with AreEqual = Some (fun _ _ -> true) })

We could also swap the position with this method so we can specify the options at the beginning like @zanaptak wanted to do the first time around.

@zanaptak
Copy link
Contributor

My own preference would be render param first followed by optional args.

React.memo(render, ?areEqual, ?name, ?withKey)

This is straightforward and consistent with React API, which is essentially memo(render, ?areEqual) -- the differences are only additive. Since we've modeled hooks to be consistent with React API -- they keep function param first -- I would favor the same for memo and do away with the requirement to put the render function last. (And same for functionComponent so they remain interchangeable minus the areEqual param.)

@cmeeren
Copy link
Contributor Author

cmeeren commented Nov 15, 2019

I'd also like to see a convenience ('props -> 'props -> bool) -> ('props -> ReactElement) function so that I can do

let MyComponent =
  React.functionComponent(fun props ->
    Html.none
   )
   |> React.withMemo memoEqualsButFunctions

without having to define the non-memoed component privately first. Though this helper is simple to define myself, so it's no biggie.

@cmeeren
Copy link
Contributor Author

cmeeren commented Nov 15, 2019

Also, I think I'm with @zanaptak on this one. I don't really want to have to update records I haven't defined myself, and the single overload with optional params is both simple and general.

@zanaptak
Copy link
Contributor

@cmeeren Ideally we don't need to use additional pipes or nesting to memoize a component -- just change functionComponent to memo and update parameters if needed.

We probably don't need to use memoEqualsButFunctions as much (if at all). Instead, React's provided technique for handling non-stable function references would be to memoize the functions in your props object with useCallback, rather than using custom equality that ignores them. Then you can just default to the built-in React equality in most cases, and only use custom equality when truly needed for special cases.


Some more thoughts on overloads.

I still agree with the overall sentiment of this ticket. There are 8 overloads for functionComponent and 16 for memo. It is difficult to get a good picture of the API when having to scroll and flip back and forth through many tooltips. Optional parameters help with this by consolidating the API into fewer self-contained overloads.

However, one problem with having parameters after an inline render function is that it makes code formatting more awkward. You have to fiddle a bit more with parentheses and indentation to get it to work when there is a trailing parameter vs. just a final closing paren. One of the draws of this library is less awkward formatting, so this prospect does not make me very "feliz".

This issue already exists for useEffect/useCallback/useMemo so one could argue it's no different for functionComponent/memo -- just figure out a working format and use it everywhere. But functionComponent/memo will be the primary building blocks of an app (besides plain functions), so one could also argue they should be streamlined as much as possible.

In light of these considerations, another alternative is to keep a dedicated overload for the "happy path" of name and render which will probably be the majority of usage. The API in this case would be:

React.functionComponent(name, render, ?withKey)
React.functionComponent(render, ?withKey)

React.memo(name, render, ?withKey, ?areEqual)
React.memo(render, ?withKey, ?areEqual)

This relegates only the somewhat niche withKey and areEqual to optional params. And they will often be inline functions just like the render function, so the awkward formatting is a necessary tradeoff when using them regardless of order. areEqual is moved last so that memo is fully a drop-in replacement for functionComponent (just change the method name, no need to reorder params). This does diverge from the React memo API, but as an optional param on all memo overloads it will always be on the tooltip and easy to discover the change.

@Zaid-Ajaj Zaid-Ajaj mentioned this issue Mar 12, 2020
8 tasks
@Shmew Shmew added enhancement New feature or request Feliz Issues related to Feliz labels May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Feliz Issues related to Feliz
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants