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

Monadic hooks #1101

Merged
merged 18 commits into from
Dec 23, 2024
Merged

Monadic hooks #1101

merged 18 commits into from
Dec 23, 2024

Conversation

rpiaggio
Copy link
Collaborator

Implements an alternative syntax for hooks, in the form of:

val component = ScalaFnComponent[Int] { props =>
  for {
    state       <- useState(props)
    _           <- useEffect(state.modState(_ + 1))
    forceUpdate <- useForceUpdate
  } yield
    <.div(
      state.value,
      <.button(^.onClick --> forceUpdate, "Click me")
    )
}

This provides a simpler syntax, while still guaranteeing safeguards to honor the rules of hooks: hook invocations are delayed and can only be invoked by ScalaFnComponent.

Source compatibility is preserved with the builder-style hooks.

Copy link
Contributor

@toddburnside toddburnside left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The syntax is a great improvement. It will be easier to use and much easier to create custom hooks. 🎊 Should Hooks.md be updated? We probably need the existing documentation for awhile for existing code, but it could be at the bottom and marked as legacy? I don't see why someone would want to develop new code with the old syntax once this is available. 😄

@rpiaggio
Copy link
Collaborator Author

The syntax is a great improvement. It will be easier to use and much easier to create custom hooks. 🎊 Should Hooks.md be updated? We probably need the existing documentation for awhile for existing code, but it could be at the bottom and marked as legacy? I don't see why someone would want to develop new code with the old syntax once this is available. 😄

Since this a beta branch, I was planning to give this a bit more of a test before officially documenting it, in case there are changes. After we have it in the wild for some time, we can decide how to document both styles.

Copy link
Owner

@japgolly japgolly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like a monadic approach was considered in the early days but there was a significant disadvantage to it. Something about safety that was lost (meaning there was a React-imposed requirement that I was able to enforce at the compiler-level with the existing approach, but would become a potential user-error with a mondaic approach)? I don't remember with enough detail. Looking at the code though, this monadic approach looks pretty good. In fact, it looks very nice. 👍

Impressive!

Only other two concerns are 1) documentation update (as you've already mentioned), and 2) binary backwards-compatibility — could I hear your position on it breaking? How concerned are you? Why / why not?

@rpiaggio
Copy link
Collaborator Author

I feel like a monadic approach was considered in the early days but there was a significant disadvantage to it. Something about safety that was lost (meaning there was a React-imposed requirement that I was able to enforce at the compiler-level with the existing approach, but would become a potential user-error with a mondaic approach)? I don't remember with enough detail.

I've been trying to remember if we considered this, but can't remember either. We probably considered it but didn't consider delaying evaluation, which is what makes it safe.

That said, you can still break the rules (eg: folding over an Option would allow you call hooks conditionally). But I think it's still safer than free-form and it's a small price to pay for the reduced syntax.

Looking at the code though, this monadic approach looks pretty good. In fact, it looks very nice. 👍

Thank you!

Only other two concerns are 1) documentation update (as you've already mentioned),

I'll add to the PR. Not sure how to handle the availability of both styles.

and 2) binary backwards-compatibility — could I hear your position on it breaking? How concerned are you? Why / why not?

If we make this part of a major release (3.0.0), it wouldn't be unexpected to have a breaking change. Since it's still source-compatible, this would have a very minor impact since it would just require recompiling downstream libraries with no need for code changes.

@rpiaggio rpiaggio requested a review from japgolly December 18, 2024 22:20
@rpiaggio
Copy link
Collaborator Author

rpiaggio commented Dec 18, 2024

Ended up renaming to Delayed, which actually describes what the type class is doing, and added a HookResult type alias to implement a nice API.

Also documented the new style as the default style for hooks, making it explicit that there are ways around the rules; while keeping the builder-style as an alternative that provide complete safety while being more verbose.

Finally, a straightforward way to wrap hook facades was added.

doc/HOOKS.md Outdated Show resolved Hide resolved
doc/HOOKS.md Show resolved Hide resolved
Copy link
Owner

@japgolly japgolly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed, feedback above

@rpiaggio rpiaggio requested a review from japgolly December 22, 2024 15:52
Copy link
Owner

@japgolly japgolly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Nice work!

@rpiaggio rpiaggio merged commit 38298a4 into topic/react18 Dec 23, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants