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

fix: type inference regression due to From<T> for Signal<T> impl (closes #3200) #3255

Closed
wants to merge 3 commits into from

Conversation

gbj
Copy link
Collaborator

@gbj gbj commented Nov 17, 2024

No description provided.

@gbj gbj mentioned this pull request Nov 17, 2024
@zakstucke
Copy link
Contributor

zakstucke commented Nov 17, 2024

As outlined in #3200 (comment), I don't think this is the right way to go.

SimpleCounter<T>(#[prop(into)] step: Signal<T>) will always be a broken pattern (as it was in 0.6 anyway inference-wise), the compiler will always have an impl conflict on some input type that should be conceptually valid.

@gbj
Copy link
Collaborator Author

gbj commented Nov 17, 2024

Have you tried SimpleCounter<T>(#[prop(into)] step: Signal<T>)? It works on 0.6, and it works on 0.7 with this PR (see here) so I'm not sure what you mean by saying it "will always be a broken pattern," unless I'm misunderstanding something.

@zakstucke
Copy link
Contributor

zakstucke commented Nov 18, 2024

will always be a broken pattern

In that it will always cause problems, with this PR you fix the example for e.g. RwSignal/Signal inputs specifically for the rare generic T in a signal prop interface, but has these 2 downsides:

  1. raw e.g. step=5 still fails, requiring step=Signal::stored(5), this was also the case in 0.6
  2. most importantly, any usages of #[prop(into)]: Signal<usize> now require a manual Signal::stored(..) for raw usize where they would otherwise not need it, which is really painful downstream as you can imagine this is predominant usage pattern.

With the solution I offer in #3200, 1. is still required and unavoidable, but 2 does not apply anymore and is solved.

(I keep mentioning rare by the way, I back that up by only having used this once myself, and looking upstream at thaw who opened the issue, they only use this pattern once in the entire lib that I could find)

@gbj
Copy link
Collaborator Author

gbj commented Nov 18, 2024

I think you're right, see my comment back on the issue :-)

@gbj gbj closed this Nov 18, 2024
@gbj gbj deleted the 3200v2 branch November 18, 2024 13:56
@zakstucke
Copy link
Contributor

Awesome, glad we hashed this one out, this PR made me nervous 😅

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