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

Clarify captures documentation #65

Open
milesfrain opened this issue Nov 20, 2020 · 1 comment
Open

Clarify captures documentation #65

milesfrain opened this issue Nov 20, 2020 · 1 comment
Labels
document me Improvements or additions to documentation

Comments

@milesfrain
Copy link
Contributor

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.

@thomashoneyman thomashoneyman added the document me Improvements or additions to documentation label Nov 25, 2020
@thomashoneyman
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
document me Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants