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

Support collecting iterators into VNode #622

Merged
merged 6 commits into from
Sep 5, 2019

Conversation

hgzimmerman
Copy link
Member

@hgzimmerman hgzimmerman commented Aug 29, 2019

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

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

To the start of this PR.

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

To now

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

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

Any feedback on how to achieve this design goal would be appreciated.

@therustmonk
Copy link
Member

Thank you for the PR! 👍

Could you try to implement FromIterator for Html/VNode? In this case we can use standard collect call instead of having a new method.

Does this change gives extra benefits to users instead of straightforward for call? If yes, we can add it. If won't than the current change actually forces users to add extra imports and replace one workaround to the other workaround.

@hgzimmerman
Copy link
Member Author

hgzimmerman commented Aug 31, 2019

Thanks. FromIterator lets me implement this "improvement" on iterator syntax more succinctly than the previously used wrapper.

I wouldn't merge this unless we can get to the aforementioned "ideal" state where you can just dump a value that happens to be convertible to an iterator over html elements into the html! macro and have it work. It would be a change that everyone would have to implement, for the minimal benefit of keeping the "jsx"-y syntax of html! without keywords.

I'm finding that the original for syntax is very useful by providing context to the macro to allow it to handle iterator transformations and just removing it will prove difficult.

Edit:
I mean to say, I wouldn't merge a fully completed version of this, with the for syntax removed. Personally for the time being, I would like to have the ability to express iterable html using collect(), despite its verbosity.
But I understand arguments against it on the basis of having two alternative ways to render lists of items can be confusing to new users and may be annoying to maintain.

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.

@jstarry
Copy link
Member

jstarry commented Sep 5, 2019

Agreed, let's keep the for syntax and use this approach until Rust gets smarter 👍

@jstarry
Copy link
Member

jstarry commented Sep 5, 2019

bors r+

bors bot added a commit that referenced this pull request 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>
@bors
Copy link
Contributor

bors bot commented Sep 5, 2019

Build succeeded

  • continuous-integration/travis-ci/push

@bors bors bot merged commit 87f1b82 into yewstack:master Sep 5, 2019
@hgzimmerman hgzimmerman changed the title Remove special 'for' syntax (WIP) Support collecting iterators into VNode Sep 14, 2019
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.

3 participants