purescript-halogen-hooks icon indicating copy to clipboard operation
purescript-halogen-hooks copied to clipboard

Clarify `captures` documentation

Open milesfrain opened this issue 5 years ago • 1 comments
trafficstars

Some proposals:

  1. Add a few words about captures to this section: https://github.com/thomashoneyman/purescript-halogen-hooks/blob/main/docs/01-Hooks-At-A-Glance.md#the-effect-hook

  2. Change the language here to clarify that captures is not an optional performance enhancement, but that it is required. (obvious in hindsight when looking at the type signature, but this might still trip-up future readers too). https://pursuit.purescript.org/packages/purescript-halogen-hooks/0.4.3/docs/Halogen.Hooks#v:captures When I originally read "Used to improve performance", I assumed this worked like Halogen.HTML.memoized, where you can optionally choose to use it. https://pursuit.purescript.org/packages/purescript-halogen/5.0.0/docs/Halogen.HTML#v:memoized

  3. Add a note about how captures is necessary in the useTickEffect docs too. https://pursuit.purescript.org/packages/purescript-halogen-hooks/0.4.3/docs/Halogen.Hooks#v:useTickEffect Also, I see "array" mentioned here a lot, but the first argument looks like a record.

milesfrain avatar Nov 20 '20 07:11 milesfrain

I agree that the captures documentation could use some improvement and with the three places you've identified as places it could see changes.

Change the language here to clarify that captures is not an optional performance enhancement, but that it is required.

This is an artifact from an older version of the code where captures produced an Array MemoValue so you could in fact provide an empty array (useTickEffect [] do ...). That's also the reason for the references to "array" rather than "record". However, in the current version you have to use captures each time, capturing an empty record if you don't have any dependencies.

I'd like to loop back around to this to update the documentation and ping you with the changes for your review.

thomashoneyman avatar Nov 25 '20 06:11 thomashoneyman