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

Option<T> read-like traits & deprecate MaybeSignal/MaybeProp #3098

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

zakstucke
Copy link
Contributor

@zakstucke zakstucke commented Oct 10, 2024

Got Option<T> working to automatically implement read-like traits for any T implementing read-like traits, so MaybeProp but generic for all RwSignal, Memo, Signal... etc.

image ^😍

I think my changes across the last few PR's now meet the requirements to deprecate MaybeSignal and MaybeProp but that's up to you.

This PR depends on #3076, so this shouldn't be merged until after that's finished with and merged.

The only actual change in this PR is the new trait_options.rs file, everything else is changes from the unmerged #3076 or deprecation stuff. Edit: and some more From<> impls for Signal.

I could implement Write-like traits too, wouldn't be hard, each op would just be a noop when None, but maybe this is too weird.

@gbj
Copy link
Collaborator

gbj commented Oct 13, 2024

So, this is objectively nice, and a good idea to implement the traits on Option<T>.

I'd note that it's different from MaybeProp, which is more like Option<Signal<Option<T>>> but flattens them into a single Option<T> when you read it, whereas using the method in this PR yields an Option<Option<T>>.

I'd like to review in a little more depth. It also seems there is some overlapping code between this and the other PR, which led to a merge issue after I merged the other one, and also some duplicate code to review... Would you mind fixing the merge issues? Then I'll take a look again.

@zakstucke
Copy link
Contributor Author

zakstucke commented Oct 13, 2024

Fixed conflicts!

You're right I was forgetting MaybeProp equated to Option<Signal<Option<T>>>, I've made some updates to provide feature parity with MaybeProp:

  • More From<> impls
  • An OptProp (plz rename haha) type alias for Option<Signal<Option<T>>>
  • A flatten() trait helper method for Option<&Option<T>>

Aside from the non StoredValue static T case, which I think is a rare enough usecase it could be defined in userland, after #3105 there should be complete feature parity with just using basic signals but the added benefit of:

  • Native setting to dom nodes e.g. see impl IntoClass for Option<impl IntoClass> #3104
  • Much improved From<> type inference, everything just comes from Signal automatically
  • Less burden inside leptos to manage all of the impls needed for MaybeSignal/MaybeProp to be as dynamic as Signal
image

@zakstucke
Copy link
Contributor Author

zakstucke commented Oct 13, 2024

But to be clear I have no issue getting rid of the deprecations by the way!

As you've seen over the last few PR's I've done, my main goal is to "elevate" top-level Option<>s to a point where MaybeSignal/MaybeProp are never needed, and ideally not recommended to component libraries anymore.

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.

2 participants