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

Only elements are allowed at the top level of view! {} macro #55

Closed
jquesada2016 opened this issue Nov 6, 2022 · 14 comments
Closed

Only elements are allowed at the top level of view! {} macro #55

jquesada2016 opened this issue Nov 6, 2022 · 14 comments
Labels
enhancement New feature or request

Comments

@jquesada2016
Copy link
Contributor

The following code is allowed:

use leptos::*;
use wasm_bindgen::prelude::*;

#[wasm_bindgen(start)]
pub fn start() {
  mount_to_body(view_fn);
}

fn view_fn(cx: Scope) -> impl Mountable {
  let (focus, set_focus) = create_signal(cx, false);

  let focused = move || {
    if focus() {
      view! { cx, <p></p> }
    } else {
      view! { cx, <span /> }
    }
  };

  view! { cx,
    <div>
      <input on:focus=move |_| set_focus(true) on:blur=move |_| set_focus(false) />
      {focused}
    </div>
  }
}

but the following is not:

  view! { cx,
    <input on:focus=move |_| set_focus(true) on:blur=move |_| set_focus(false) />
    {focused}

  }

I do understand why, because the macro expects these items to be Element rather than impl IntoElement, but thought it was worth mentioning in case this was not intentional.

@gbj
Copy link
Collaborator

gbj commented Nov 6, 2022

Yeah the macro doesn't have any opinion about the type of what it's returning, although if you have multiple root nodes it should be wrapped in a fragment (<></>)

The bigger issue here would be the type of what you're returning: if you had two elements you could return a Vec, but you have one Element and one Fn() -> Element. What would you hope the type would be that you're returning there?

@jquesada2016
Copy link
Contributor Author

jquesada2016 commented Nov 7, 2022

I'm not sure how it works under the hood, but I would expect the macro to call IntoChild::into_child(/* .. */) on each root element and then convert the children into an Element, this way it would always return either Element or Vec<Element>.

@gbj
Copy link
Collaborator

gbj commented Nov 7, 2022

Yeah that would be one possible approach... It would return Child rather than Element (because it could be a text node, a Vec<Node>, etc.) You'd have to match on that Child to get access to the underlying DOM element.

The Element type is literally a web_sys::Element so in your example {focused} can never be an Element; it's not an Element, but a function that returns an Element. It can be a Child, but that's basically an internal type for the renderer.

I guess it would be possible to return each node as an enum of all the possible node types, but I kind of feel like that would be worse for most use cases.


Sidebar: I know it's just a simple example, but in the example it's probably actually a better idea to do something like

let focused = move || {
    if focus() {
      "focused"
    } else {
      "not focused"
    }
  };


  view! { cx,
    <div>
      <input on:focus=move |_| set_focus(true) on:blur=move |_| set_focus(false) />
      <p>{focused}</p>
    </div>
  }

Because this causes only the text node inside the p to change when focus() changes, rather than replacing a whole element. This is one of the reasons wrapping something in an element isn't a big deal in most cases... it's often the more efficient way to do things anyway!

@jquesada2016
Copy link
Contributor Author

I think having an opaque type returned by view! {} is not a big deal, as long as there exists a way of getting the element, such as adding a create_ref which would allow one to do something like <div ref=ref_ />.

I had originally structured the example like that on purpose to also demonstrate that I didn't see an easy way of returning an empty view, which is very common for UI component libs as they often have to toggle between there being a node, and there not being a node, such as in the following example:

let helper_text = view! { cx,
  if some_signal() {
    view! { <label>The help is on the way</label> }
  } else { view! { cx, } }
};

view! { cx,
  <div class="form-control">
    <label>"The input"</label>
    <input />
    {helper_text}
  </div>
}

@gbj
Copy link
Collaborator

gbj commented Nov 7, 2022

In this specific case, what you probably want is to use an Option, so something more like

let helper_text = move || some_signal().then(|| {
  view! { cx, <label>The help is on the way</label> }
});

view! { cx,
  <div class="form-control">
    <label>"The input"</label>
    <input />
    {helper_text}
  </div>
}

This actually works fine because Fn() -> Option<C> is implemented where C: IntoChild.

@gbj gbj added the enhancement New feature or request label Nov 7, 2022
@jquesada2016
Copy link
Contributor Author

That is correct, that is much better and what I did want to originally achieve.

This, however, doesn't seem to be possible with the macro, perhaps you can shed light on how to achieve the same result without relying on wrapping with a <span />? This is a common pattern for context providers:

fn Provider(cx: Scope) -> Element {
  // ...
  // Init the provider stuff
  // ...

  let children = move || {
    is_initialized().then(|| {
      provide_context(cx, ProvideCtx(/* ... */));

      children()
    })
  };

  view! { cx,
    <span>  // <-- ideally, remove these
      {children}
    </span> // <-- ideally, remove these
  }
}

I know the pattern above lends itself to be async, and thus should probably use Suspense, but the problem is the same; there's no way to return Fn() -> Option<Element> as in your example without first wrapping with a dummy <span />.

A potential solution would be to not use the macro and change the fn signature to return impl Fn() -> Option<Element>, and then use this provider as a child of something else, but this just bubbles the problem up, eventually you'll need to have a <div> or <span> wrapping the provider, which isn't too much of a big deal, but not ideal.

@jquesada2016
Copy link
Contributor Author

jquesada2016 commented Nov 7, 2022

I found a workaround which seems to work in the meantime.

If we create a dummy component:

#[component]
pub fn Fragment(cx: Scope, children: Box<dyn Fn() -> Vec<Element>>) -> Vec<Element> {
  children()
}

And then we use this in the view! {} macro, it works without needing to create any wrapper elements.

view! {
  <Fragment>
     /* do your thing */
  </Fragment>
}

@gbj would this be a potential solution for this issue? To add a Fragment component to the lib and have the macro create a Fragment when using <></>?

Edit: No, I see it does not, because the fragment returns a Vec<Element>, but would actually need to return Element in order to work.

@gbj
Copy link
Collaborator

gbj commented Nov 7, 2022

Re: the context provider example: Can't you achieve exactly what you're trying to achieve by simply returning children? The view macro doesn't have any secret sauce other than creating HTML/DOM elements and making sure they are reactively updated. For a context provider, I'd assume you're just trying to do some work to provide that context and pass through the children; you can just return children.


More generally: leptos::Element is a type alias for web_sys::Element. If you want to return something more generic from your component functions, you can always return impl IntoChild or even Child (by returning view! { cx, ... }.into_child(cx).

Is the goal with the Fragment component to solve the original problem of being able to (basically) return a heterogeneous list without a wrapping DOM element? In that case what you really need to return is probably Vec<Child>, by returning something like children().into_iter().map(move |child| child.into_child(cx)).collect().

@jquesada2016
Copy link
Contributor Author

This almost works, except for two situations.

  1. Mountable is not implemented for Child, therefore you always need at least a top level wrapping element.
  2. Components receive a Box::new(something), so I'm not sure if it would allow receiving children of mixed types? Mixed types being Element, Vec<Element>, and Child

I presume point 2 is nil, but I play around with it and report back. 1. for sure doesn't work.

@gbj
Copy link
Collaborator

gbj commented Nov 8, 2022

  1. Hm yeah. I had not actually thought of this before, but if you really wanted to mount a Child to the DOM as the root, you could call leptos::insert(cx, parent, child, Marker::NoChildren, None). insert is basically an internal function but I can add some docs as it would be useful for a case like this.
  2. Yeah the children prop is pretty much bound to Box::new(dyn Fn() -> Vec<T>) — I've messed around with it a lot and this is kind of the best option to hydrate things in the correct order and allow a certain amount of generic-ness. For example the router allows a <Routes> component that takes <Route> children, which return RouteDefinition and not any kind of element type.

You're right that this prevents you from doing that Fragment component yourself.

@jquesada2016
Copy link
Contributor Author

Why not impl IntoChild for Vec<Child> and have components always return Child instead of Element or Vec<Element>? Then just provide a helper method on Child which would return the closest web_sys::Node. This would require only implementing Mountable only for Child and simplifies the API, I think.

@gbj
Copy link
Collaborator

gbj commented Nov 8, 2022

I'd be open to doing this in the macro in certain circumstances: for example, if you're creating a fragment, and not every item is an element (so, your first example). This can be detected at macro time, and allows returning Vec<Child> in this case while sticking with Element or Vec<Element> when those are known to be true.

The larger issue, believe it or not, is that Child doesn't actually have a Child::Children variant. At the moment the renderer can handle things if a Child is a Vec<Node> but not a Vec<Child>.

https://github.com/gbj/leptos/blob/6b82a37dea7a77d5e473f9151534c8642ff4e5f7/leptos_dom/src/child.rs#L10-L16

The whole DOM renderer is built on this type so I'll have to think about how it would work to handle it recursively.

I think this may be a good compromise solution.

@gbj
Copy link
Collaborator

gbj commented Nov 23, 2022

Okay I just had an idea: if the fragment syntax <></> returns an actual DocumentFragment, it should actually be possible to mix and match types in exactly the way we want to. DocumentFragment is a Node so I'm pretty sure it implements IntoChild all on its own.

I'm going to play around with that today and see if I can come up with a good solution that doesn't involve introducing another type and more runtime complexity.

@gbj
Copy link
Collaborator

gbj commented Dec 9, 2022

I'm going to close this issue for now... This is clearly what we're doing with #116 and we can archive this discussion.

@gbj gbj closed this as completed Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants