-
-
Notifications
You must be signed in to change notification settings - Fork 186
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
Expanding the documentation #463
base: master
Are you sure you want to change the base?
Conversation
WalkthroughThe pull request updates documentation within the Flutter Hooks framework. Changes include revisions to the comments for various classes and methods (e.g., Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
/// [Hook] is similar to a [StatelessWidget], but is not associated | ||
/// to an [Element]. | ||
/// Allows a [Widget] to create and access its own mutable data | ||
/// without implementing a [State]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that "not associated to an Element" might lead to confusion, because much like a StatefulWidget
, Hook
has a createState()
method, and the object returned has access to the context
.
/// A [Hook] is typically the equivalent of [State] for [StatefulWidget], | ||
/// with the notable difference that a [HookWidget] can have more than one [Hook]. | ||
/// A [Hook] is created within the [HookState.build] method of a [HookWidget] and the creation | ||
/// must be made unconditionally, always in the same order. | ||
/// Whereas [Widget]s store the immutable configuration for UI components, | ||
/// [Hook]s store immutable configuration for any type of object. | ||
/// The [HookState] of a [Hook] is analogous to the [State] of a [StatefulWidget], | ||
/// and a single [HookWidget] can use more than one [Hook]. | ||
/// | ||
/// Hooks can be used by replacing `extends StatelessWidget` with `extends HookWidget`, | ||
/// or by replacing `Builder()` with `HookBuilder()`. | ||
/// | ||
/// Hook functions must be called unconditionally during the `build()` method, | ||
/// and always in the same order. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A [Hook] is typically the equivalent of [State]
This makes sense, since you can go by the philosophy of "rather than making a State, make a Hook instead".
However, in my mind, the equivalent of State
would be HookState
rather than Hook
.
My hope is that the adjusted wording creates a good intuition while avoiding this caveat.
A [Hook] is created within the [HookState.build] method of a [HookWidget]
I believe this sentence was a typo: a HookState
is created by a Hook
, not the other way around.
/// Called before a [build] triggered by [markMayNeedRebuild]. | ||
/// | ||
/// If [shouldRebuild] returns `false` on all the hooks that called [markMayNeedRebuild] | ||
/// then this aborts the rebuild of the associated [HookWidget]. | ||
/// | ||
/// There is no guarantee that this method will be called after [markMayNeedRebuild] | ||
/// was called. | ||
/// Some situations where [shouldRebuild] will not be called: | ||
/// If [shouldRebuild] returns `false` on all the hooks that called [markMayNeedRebuild], | ||
/// [HookElement.build] will return a cached value instead of rebuilding each [Hook]. | ||
/// | ||
/// - [setState] was called | ||
/// - a previous hook's [shouldRebuild] returned `true` | ||
/// - the associated [HookWidget] changed. | ||
/// This method is not evaluated if a previous Hook called [markMayNeedRebuild] | ||
/// and its [shouldRebuild] method returned `true`. | ||
/// Additionally, if [setState], [didUpdateHook], or [HookElement.didChangeDependencies] is called, | ||
/// the build is unconditional and the `shouldRebuild()` call is skipped. | ||
bool shouldRebuild() => true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the original description was really good, but I saw that (1) a rebuild will happen anyway if "a previous hook's shouldRebuild
returned true
" and (2) the method returns true
by default… and for some reason I totally missed that only the hooks that called markMayNeedRebuild
are checked.
I also saw that the 3 bullet points technically didn't cover every possibility, since an InheritedWidget
could also trigger an unconditional rebuild.
/// A [Widget] that can use a [Hook]. | ||
/// A [Widget] that can use [Hook]s. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My hope was to tweak the wording here, so it's more explicit that you can use multiple Hooks.
/// Generally, Hook functions must be called unconditionally, in the same order. | ||
/// That rule does not apply to `useContext()`, however, since instead of accessing a [Hook], | ||
/// it merely returns the relevant [BuildContext]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see an argument for calling useContext()
unconditionally just for consistency, but since it can make things slightly less readable, I think it's nice to include the information here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not tell people to do violate hooks rules with useContext
specifically.
If we want to allow useContext
to not respect those rules, then it shouldn't be named with the useX
convention.
IMO diognistic tools could look for use*
inside Widget.build
and expect all of them to follow hook rules. I'd rather not have exceptions for this ; even if technically doable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO diognistic tools could look for
use*
insideWidget.build
and expect all of them to follow hook rules.
That's a good point. I still think it'd be nice for developers who read these docs to have access to this info, but I agree that it shouldn't be recommended. I'll try to adjust the wording here accordingly.
Quick update—I've reverted the changes to the Hopefully now we can just focus on the uncontroversial changes here. I especially like the implementation of |
I've been doing things with Hooks a lot over the past few months, and it's been really fun to learn about the inner mechanisms of flutter_hooks.
The goal of this PR is to flesh out a few of the API descriptions and to tweak the wording in a few places for better accuracy.
Summary by CodeRabbit
Hook
class to clarify its functionality and usage.createState
andshouldRebuild
methods to enhance understanding.HookState
andHookElement
in managing hook states.HookWidget
andStatefulHookWidget
to highlight their similarities to standard widgets.useContext
function's behavior and error handling.