Function Components & Hooks V2

[futursolo]

Description

Fixes #2364
Fixes #2343
Fixes #2026

Summary of Changes:

• Hooks must be marked as hook with #[hook].
• Hooks are now limited to be called inside of #[function_component] & #[hook].
• Hooks can only be called in order and in correct calling position.
• Hooks are required to have a use_ prefix.
• Rendering State (HookContext) is acquired once throughout the entire rendering cycle and no longer uses thread locals.
• use_hook has ceased to exist.
• use_ref is renamed to use_memo and re-calculates its value when the dependency change.
• Hooks are now allocation free on subsequent renders if that hooks does not change / update.

Still Missing:

• Macro-based Hooks support (for a later pull request)

Checklist

• I have run cargo make pr-flow (cargo make pr-flow is broken on macOS? pr-flow is broken on macOS #2403)
• I have reviewed my own code
• I have added tests

[github-actions]

Visit the preview URL for this PR (updated for commit 806f618):

https://yew-rs--pr2401-fc-hook-v2-ziafg6gv.web.app

_{(expires Thu, 03 Feb 2022 14:27:51 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

[voidpumpkin]

Works for me

[WorldSEnder]

First off, I like the push towards linting for hooks.

Many of my review comments stem from the simple fact that I don't think function components are the end-all-be-all of design. With BaseComponent being sealed, there seems to be some serious push towards building a walled-garden and I don't see how that's a good thing:

• Anyone writing hooks is subjected to the syntax-based rewriting of the #[hook] attribute. Instead of exposing the functionality by a clean interface, such as a trait, it's exposed under the opaqueness of a proc-macro and can't be accessed otherwise. Protecting the user from himself beside - the usual mechanics of saying "I know what I'm doing, let me peek under the hood" of rust - unsafe functions - can't be used.
• Most of this is based on making use_hook private. Are you saying that it's impossible somebody outside yew to correctly use it? If yes, then I'd like to hear more reasons why. If no, I wonder why that method can't be marked unsafe or similar guarded with warnings about what exactly is "unsafe" about using it directly.
• By making use_hook private, anybody writing custom hooks has to, at some point, call one of the Pre-defined Hooks, there's no way around that. I can't agree with that until I see some good reason that the pre-defined hooks are sufficient for any but the most fringe applications.

Besides those concerns, I had a quick look at the code, but skipped the syntax transformations in the hook proc macro for now.

[WorldSEnder]

I take it that this design means I can't call any function starting with use_ that is not a hook and false positives are a conscious trade-off? How do I turn this off when such a false positive occurs?
I have some doubts that this kind of syntax guided rewriting is maintainable in the long term, do you know any bigger library that uses this approach successfully to convince me that it's not going to break on us in the future?

[futursolo]

How do I turn this off when such a false positive occurs?

use some_crate::{use_something as not_a_hook} ;

that uses this approach successfully

This is used in std as well, which I will reply to the main comment instead.

[voidpumpkin]

@futursolo we should mention this in the error to be more user friendly

[futursolo]

There's no way to do it. The procedural does not know the type of your expression.

The only error will be from the compiler saying that use_something does not implement Hook.

[futursolo]

First off, I like the push towards linting for hooks.

Many of my review comments stem from the simple fact that I don't think function components are the end-all-be-all of design. With BaseComponent being sealed, there seems to be some serious push towards building a walled-garden and I don't see how that's a good thing:

Do you think #![deny(unsafe_code)] is a good thing?

• Anyone writing hooks is subjected to the syntax-based rewriting of the #[hook] attribute. Instead of exposing the functionality by a clean interface, such as a trait, it's exposed under the opaqueness of a proc-macro and can't be accessed otherwise.

The #[hook] has mirrored the design of the async / await syntax from the standard library.
At one point, async function was even implemented as #[async_fn].

If you are confident with using async / await, you should be confident with using this syntax.

push towards building a walled-garden

This is always the case in Rust?

can't be accessed otherwise

How do you poll a future manually inside a future?

Short answer, It's not possible (as std::task::Context is not available).
Long answer, It's not possible without blocking the current thread. (You can write a runtime that blocks current thread that manually polls the future.)

Protecting the user from himself beside - the usual mechanics of saying "I know what I'm doing, let me peek under the hood" of rust - unsafe functions - can't be used.

What would an unsafe fn accomplish if a hook is unsafe? It does nothing than requiring a unsafe block being around it.
You can have unsafe code inside of a function even if it's not unsafe.

• Most of this is based on making use_hook private. Are you saying that it's impossible somebody outside yew to correctly use it? If yes, then I'd like to hear more reasons why. If no, I wonder why that method can't be marked unsafe or similar guarded with warnings about what exactly is "unsafe" about using it directly.

use_hook is actually subject to removal. I only kept this so all built-in hooks can work as-is with only #[hook] applied.

• By making use_hook private, anybody writing custom hooks has to, at some point, call one of the Pre-defined Hooks, there's no way around that. I can't agree with that until I see some good reason that the pre-defined hooks are sufficient for any but the most fringe applications.
  Hooks, there's no way around that.

There's no way around that in React either. If you don't need use_hook in React, you shouldn't need use_hook in Yew either.

[WorldSEnder]

Do you think #![deny(unsafe_code)] is a good thing?

Yes, in some code, but probably not here. Maybe with an additional feature but I don't think it's feasible for yew. What's that got to do with making it impossible for library code to emulate FunctionComponent and other functionality (like generator based components that could work with suspense), anyway?

EDIT: An explicit example, I had generator components working. I could see a very simple change to change them to be able to suspend, just changing Html to HtmlResult, yet I can't impl BaseComponent in library code any more. Walled off, for no particular reason. That's the kind of walled garden I'm talking about.

At one point, async function was even implemented as #[async_fn].

And yet, the actual implementation that shipped in stable is much more deeply engrained in the language grammar and a larger part of the compiler. That doesn't really convince me that rebuilding such a transformation is a smart idea.

How do you poll a future manually inside a future?

You can use Context::from_waker, build a Waker from a RawWaker (unsafe, but accessible). It's got documented unsafety guarantees you need to uphold, but you can more or less straight forward do that. No need to block the thread, but it's very involved. But the whole async implementation is just way more involved than the hooks interface in yew.

There's no way around that in React either. If you don't need use_hook in React, you don't need use_hook in Yew either.

That'd be a fine sentiment if this were simple a React clone. Alas, blindly trodding the path of Facebook is not really the approach I'd like to see.

use_hook is actually subject to removal.

Haven't seen that, can you link the relevant issue and discussion? The only open issue/discussion mentioning it is this PR. Losing it would mean losing access to HookUpdater, again, for no reason I can see, the API seems fine.

[futursolo]

Walled off, for no particular reason.

I don't think Yew is an elite hacker toy for one to play with.
This is done to encourage good, established design patterns to minimise the number of issues developers may having.
This also allows BaseComponent's API to change more rapidly to add more functionality without users having to update every component.

And yet, the actual implementation that shipped in stable is much more deeply engrained in the language grammar and a larger part of the compiler. That doesn't really convince me that rebuilding such a transformation is a smart idea.

async and await are nothing more than syntax sugars and I cannot remember anything that indicates the same cannot be done with procedural macros.
As far as I remember, the decision of making them syntax was due to:

1. async blocks like async move {} is not possible with procedural macro so we give this to function as will for sake of consistency.
2. a().await.b().await.c().await looks cleaner than await!(await!(await!(a()).b()).c())

Neither of these applies to hooks.

You can use Context::from_waker, build a Waker from a RawWaker (unsafe, but accessible). It's got documented unsafety guarantees you need to uphold, but you can more or less straight forward do that. No need to block the thread, but it's very involved. But the whole async implementation is just way more involved than the hooks interface in yew.

You cannot do any async IO without the access to the original context unless you await on it.
If you don't have the original context, you cannot wake up the current task.
In addition, you cannot return Poll::Pending manually from an async fn to suspend the current task.

Haven't seen that, can you link the relevant issue and discussion? The only open issue/discussion mentioning it is this PR. Losing it would mean losing access to HookUpdater, again, for no reason I can see, the API seems fine.

use_hook is merely a wrapper around HookContext::next_state and can be removed to improve code size.

If a component is suspendible, then consider the following code:

use_a_low_level_hook();
use_a_suspendible_hook()?;

For any hook before a suspension, callback will be called immediately and post_render will be delayed until after the component is no longer suspended and the post_render will be registered as many times as the view function is called.
If one doesn't understand what they are doing, it's very easy to write a hook that can deadlock the component.
(e.g.: use callback returns true to render the component until a state condition is met and that state condition is updated in post_render. This would be fine if a component is not suspended, but not would deadlock if a component is suspended and would be hard to spot.)

I don't (and I believe most maintainers don't either) have time to explain to everyone about why their new low-level hook with HookContext does not work as intended when you can just use built-in hooks and it would work as expected. (including this comment.)

This is going to be the case for all components if Yew eventually switches to a concurrent renderer. (view() is called multiple times and its result becomes discardable until all messages are drained and achieves a consistent state across all components.)

In addition, the post_render call is not called on SSR but callback is and callback calls does not reliably re-render even if it returns true unless currently suspended.

TL; DR

In short, I don't think these objections were done based on legit ground / issues that already demonstrated the limitation of the design but speculations around potential possibility of the limitation of either some issue may surface or a new design may supersede function component. Potential bugs happen with any design and it needs time for it to become polished.

[ranile]

Many of my review comments stem from the simple fact that I don't think function components are the end-all-be-all of design. With BaseComponent being sealed, there seems to be some serious push towards building a walled-garden and I don't see how that's a good thing:

Is there a case where you need BaseComponent and can't use Component? According to my understanding, it's merely a wrapper around Component to do some type system magic with the return type of view method to allow writing cleaner code. If that is not the case, I'm in favor of exposing of BaseComponent.

We should allow library authors to add different types of components (like generator components). That path shouldn't be closed.

What would an unsafe fn accomplish if a hook is unsafe? It does nothing than requiring a unsafe block being around it.
You can have unsafe code inside of a function even if it's not unsafe.

unsafe denotes, "I know what I'm doing and I uphold the guarantees required to make this code sound/safe/correct". I think that is relevant here.

use_hook is no longer exported. (The methods it provides are not safe in SSR and Hydration for a reason similar to struct components. Users should use a combination of built-in hooks instead.)

This is exactly what unsafe should specify.

use_hook is actually subject to removal.

Haven't seen that, can you link the relevant issue and discussion? The only open issue/discussion mentioning it is this PR. Losing it would mean losing access to HookUpdater, again, for no reason I can see, the API seems fine.

Why not just remove it now? HookUpdater can be exposed with an unsafe API where the safety guarantees are clearly documented.

I must admit, I mostly glanced over the code. I'll review in more in depth soon.

[futursolo]

We should allow library authors to add different types of components (like generator components).

I don't disagree with this but BaseComponent should be provided with no guarantee (if it is provided).

In addition, I highly doubt a stateful API at render time (generator component) would do any good.

If we were to give Yew Concurrent Mode, then view() needs to become stateless and side-effect free (like React).
As it will potentially be called many times before a render will happen (only the last returned VNode will be rendered.)

unsafe denotes, "I know what I'm doing and I uphold the guarantees required to make this code sound/safe/correct". I think that is relevant here.

unsafe is used on when memory safety is not guaranteed and not anything "use at your own risk".

https://doc.rust-lang.org/std/keyword.unsafe.html

Code or interfaces whose memory safety cannot be verified by the type system.

https://doc.rust-lang.org/std/future/trait.Future.html

std has documentation that describes Future::poll's potential hazard which provides a good explanation:

Panics

Once a future has completed (returned Ready from poll), calling its poll method again may panic, block forever, or cause other kinds of problems; the Future trait places no requirements on the effects of such a call. However, as the poll method is not marked unsafe, Rust’s usual rules apply: calls must never cause undefined behavior (memory corruption, incorrect use of unsafe functions, or the like), regardless of the future’s state.

I think that is relevant here.

There's actually a limitation on the trait as the trait method is safe.
If a hook is allowed to be marked as unsafe, it requires a unsafe {} block being applied and then marked as unsafe again.
Which I have no idea what this would accomplish so I am not willing to write extra code to cater for it unless somebody actually complains about it and has a legitimate use case.

Why not just remove it now?

Because I am lazy? Keeping use_hook at the moment means that built-in hooks can work as-is and I don't have to define custom types for built-in hooks that uses it directly. (And it helps me to debug #[hook] macro.)
use_hook will be removed in an upcoming PR (hopefully in a couple weeks), I am just too lazy to remove it in this one.
The HookUpdater can potentially be removed alongside use_hook.

In addition, the low-level API needs to be provided for a good reason. I don't think there's any circumstances that you need the low level API and it's not achievable with the built-in hooks. (callback with true (re-render) -> state.set(()), post_render -> use_effect).

[ranile]

We should allow library authors to add different types of components (like generator components).

I don't disagree with this. But I highly doubt a stateful API at render time (generator component) would do any good.
If we were to give Yew Concurrent Mode, then view() needs to become stateless and side-effect free (like React).
As it will potentially be called many times before a render will happen (only the last returned VNode will be rendered.)

I agree here. Let's not expose the APIs, at least not now.

unsafe denotes, "I know what I'm doing and I uphold the guarantees required to make this code sound/safe/correct". I think that is relevant here.

unsafe is used on when memory safety is not guaranteed and not anything "use at your own risk".
[...]

I think that is relevant here.

There's actually a limitation on the trait as the trait method is safe.
If a hook is allowed to be marked as unsafe, it requires a unsafe {} block being applied and then marked as unsafe again.
Which I have no idea what this would accomplish so I am not willing to write extra code to cater for it unless somebody actually complains about it and has a legitimate use case.

We should properly document API that may end up in causing surprising behavior/headache for consumer. I was wrong about unsafe so let's not use it.

Why not just remove it now?

Because I am lazy?

That's fair. It's fine as it is too

In addition, the low-level API needs to be provided for a good reason. I don't think there's any circumstances that you need the low level API and it's not achievable with the built-in hooks. (callback with true (re-render) -> state.set(()), post_render -> use_effect).

Agreed. We should not be against exposing the APIs though so if someone comes up a valid use case, we should provide the API required for it.

In any case, I'll try to review this either today or tomorrow. Hopefully we can get merged soon.

[futursolo]

This Pull Request now no longer includes a use_hook implementation.

This eliminates heap allocations from subsequent renders in hooks if that hook is not updated / changed and reduces code size comparing to master.

[ranile]

Looks good to me. Thanks for your PRs and helping push Yew forward.