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

html!: Remove the special for syntax when rendering iterators #606

Closed
jstarry opened this issue Aug 19, 2019 · 2 comments
Closed

html!: Remove the special for syntax when rendering iterators #606

jstarry opened this issue Aug 19, 2019 · 2 comments

Comments

@jstarry
Copy link
Member

jstarry commented Aug 19, 2019

Problem

It's not intuitive to use a special syntax for rendering iterators, especially when that syntax uses a rust keyword 😝

Proposed

Implement Into<VNode> for IntoIterator<Into<VNode>>

Before

html! {
   for self.props.items.iter().map(renderItem)
}

After

html! {
   self.props.items.iter().map(renderItem)
}
@hgzimmerman
Copy link
Member

hgzimmerman commented Aug 29, 2019

I've attempted to get started with this, but I'm encountering conflicting implementation errors with the T: ToString impl.

The following code:

impl<COMP, T, U> From<T> for VNode<COMP>
where
    COMP: Component,
    T: IntoIterator<Item = U>,
    U: Into<VNode<COMP>>
{
    fn from(_: T) -> Self {
        unimplemented!()
    }
}

produces the error:

error[E0119]: conflicting implementations of trait `std::convert::From<_>` for type `virtual_dom::vnode::VNode<_>`:
   --> src/virtual_dom/vnode.rs:111:1
    |
98  |   impl<COMP: Component, T: ToString> From<T> for VNode<COMP> {
    |   ---------------------------------------------------------- first implementation here
...
111 | / impl<COMP, T, U> From<T> for VNode<COMP>
112 | | where
113 | |     COMP: Component,
114 | |     T: IntoIterator<Item = U>,
...   |
119 | |     }
120 | | }
    | |_^ conflicting implementation for `virtual_dom::vnode::VNode<_>`

Without the ability to specify negative traits or use trait specialization, I don't see an immediate way forward without modifying other code.

Would removing the T: ToString impl and replacing it with concrete implementations for String, &str, Cow<&str> [1] be an acceptable way forward?

[1] https://doc.rust-lang.org/alloc/string/trait.ToString.html#implementors


Edit:
Its harder than I thought.
I get another error if i just impl From<String> for VNode<COMP> on the basis that
upstream crates may add new impl of trait `std::iter::Iterator` for type `std::string::String` in future versions

bors bot added a commit that referenced this issue Sep 5, 2019
622: Remove special 'for' syntax (WIP) r=jstarry a=hgzimmerman

Attempts to address #606

I'm running into coherence issues and conflicting implementations with `ToString` and `IntoIterator` when I try to do a blanket impl for `impl <T: IntoIterator<etc...> From<T> for VNode<COMP>`. If I replace the `ToString` blanket impl with concrete impls for `String` and `&str`, I still run into coherence issues where downstream crates might provide a conflicting impl.

I figured that the easiest path forward is to create my own iterator and implement `Into<VNode<COMP>>` for it via an impl for `From`.

So this PR thusfar gets us from:
#### Before
```rust
html! {
   for self.props.items.iter().map(renderItem)
}
```

#### To the start of this PR.
 ```rust
html! {
   self.props.items.iter().map(renderItem).html()
}
```

#### To now
 ```rust
html! {
   self.props.items.iter().map(renderItem).collect::<Html<Self>>()
}
```

Which strictly speaking, is _better_, as it frees up a keyword, but isn't anywhere near what you ideally want:
#### Ideal
```rust
html! {
   self.props.items.iter().map(renderItem)
}
```
-------
Any feedback on how to achieve this design goal would be appreciated.


Co-authored-by: Henry Zimmerman <zimhen7@gmail.com>
@jstarry
Copy link
Member Author

jstarry commented Sep 26, 2019

From #622

I don't see the ideal case being possible without trait specialization or mutually exclusive traits landing first, which I'm guessing would take a year at the earliest.

Closing due to Rust limitation

@jstarry jstarry closed this as completed Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants