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

xilem_web: Is it possible for a update function to return two dom nodes without a parent contianer? #461

Closed
casey opened this issue Jul 28, 2024 · 10 comments · Fixed by #482

Comments

@casey
Copy link
Contributor

casey commented Jul 28, 2024

I have an app I'm rewriting using xilem_web whose HTML was formerly:

<body>
  <nav></nav>
  <main></main>
</body>

With xilem_web, I'm doing:

fn update(state: &mut State) -> impl DomView<State> {
  div((html::nav(()), html::main(())))
}

Which results in an extra wrapper div:

<body>
  <div>
    <nav></nav>
    <main></main>
  </div>
</body>

I tried:

fn update(state: &mut State) -> impl DomView<State> {
  (html::nav(()), html::main(()))
}

As well as fooling around with xilem_core::ViewSequence, to no avail. Is it possible to avoid the extra wrapper div? It's not a huge deal, but an extra wrapper div does sometimes make CSS more complex (body > div > nav instead of body > nav) and is an extra div to manually expand when inspecting the DOM.

@Philipp-M
Copy link
Contributor

Hmm yeah thinking about it, it would make sense to be able to add a "fragment" (aka ViewSequence) to the root element. Maybe I should look into that.
Though that would require an additional generic parameter for the App for the SeqMarker of a ViewSequence. And app_logic would need to be a closure to be able to return a ViewSequence that is not a DomView, so that Rusts type inference figures out the type of the SeqMarker, because with the current design we can't easily return a ViewSequence for explicitely typed functions, because of mentioned SeqMarker, which unfortunately is necessary to avoid ambiguity between View and ViewSequence (alternative solution was #162, which may still be possible but has the disadvantage of having a ViewMarker trait to be implemented for all types that implement View):

I'm also wondering, whether we should support elements such as body() directly in xilem_web (not sure yet if that has some other implications...)

@casey
Copy link
Contributor Author

casey commented Jul 29, 2024

I'm also wondering, whether we should support elements such as body() directly in xilem_web (not sure yet if that has some other implications...)

Supporting body and allowing the rendered view to replace the containing element, instead of be placed inside of it would work work for my specific use-case. It might not be worth it though if it adds complexity, and it doesn't solve the more general use-case of returning multiple sibling elements with no parent.

@DJMcNab
Copy link
Member

DJMcNab commented Jul 29, 2024

We should be able to have a fragment or root view which accepts multiple elements and adds them as siblings next to each other. That should mean that the app_logic could still return a single View.

This is similar to how the planned root for Xilem will be application, which will be able to contain multiple window views.

It would probably also be possible to have a rooted view, which places its children as children of a specific DOM node (e.g. by id?), although maybe that's too imperative.

@Philipp-M
Copy link
Contributor

It might not be worth it though if it adds complexity

I don't think it really adds complexity (at least less than allowing a ViewSequence in App) if it has no other implications I'm not seeing yet (more of browser compatibility nature than implementing that, I think that may be done in 2 lines or so...).

We should be able to have a fragment or root view which accepts multiple elements and adds them as siblings next to each other. That should mean that the app_logic could still return a single View.

Funny I started writing something yesterday suggesting something similar, but forgot to press send (possibly because it was way too late)^^

I guess that would mean a View with a different ViewElement, something like Fragment with impl SuperElement<Pod> for Fragment (Pod is the ViewElement of DomView). Yeah I guess that would make sense, that would I think also make it possible to return ViewSequences in general and be used inside all those element views. Hmm I think I'll try to get to that soon.

github-merge-queue bot pushed a commit that referenced this issue Aug 5, 2024
)

Should fix #461. This allows a `ViewSequence` (called `DomFragment`) of
`DomView`s as root component.

The `counter` example is updated to show this new behavior.
@casey
Copy link
Contributor Author

casey commented Aug 5, 2024

I ran into a small issue with this. I can now return multiple nodes, which is great:

fn foo(state: &mut State) -> impl DomFragment<State> {
  use html::*;

  (div(()), div(()))
}

However, I cannot do so from within a call to fork:

fn bar(state: &mut State) -> impl DomFragment<State> {
  use html::*;

  fork(
    (div(()), div(())),
    memoized_await((), |()| async {}, |state: &mut State, _| {}),
  )
}

I get the following error:

    Checking library-viewer v0.0.0 (/Users/rodarmor/src/gossamer/crates/library-viewer)
error: could not compile `library-viewer` (bin "library-viewer") due to 2 previous errors; 1 warning emitted
error[E0277]: the trait bound `(xilem_web::elements::html::Div<_, _>, xilem_web::elements::html::Div<_, _>): View<State, (), ViewCtx, Box<(dyn xilem_web::Message + 'static)>>` is not satisfied
   --> crates/library-viewer/src/main.rs:323:30
    |
323 | fn bar(state: &mut State) -> impl DomFragment<State> {
    |                              ^^^^^^^^^^^^^^^^^^^^^^^ the trait `View<State, (), ViewCtx, Box<(dyn xilem_web::Message + 'static)>>` is not implemented for `(xilem_web::elements::html::Div<_, _>, xilem_web::elements::html::Div<_, _>)`, which is required by `Fork<(xilem_web::elements::html::Div<_, _>, xilem_web::elements::html::Div<_, _>), MemoizedAwait<State, _, (), {closure@crates/library-viewer/src/main.rs:328:24: 328:28}, (), {closure@crates/library-viewer/src/main.rs:328:39: 328:61}, {async block@crates/library-viewer/src/main.rs:328:29: 328:37}, ()>>: DomFragment<State>`
    |
    = help: the following other types implement trait `View<State, Action, Context, Message>`:
              <&'static str as View<State, Action, Context, Message>>
              <(dyn AnyView<State, Action, Context, Element, Message> + 'static) as View<State, Action, Context, Message>>
              <(dyn AnyView<State, Action, Context, Element, Message> + Send + 'static) as View<State, Action, Context, Message>>
              <(dyn AnyView<State, Action, Context, Element, Message> + Send + Sync + 'static) as View<State, Action, Context, Message>>
              <(dyn AnyView<State, Action, Context, Element, Message> + Sync + 'static) as View<State, Action, Context, Message>>
              <Abbr<State, Action> as View<State, Action, ViewCtx, Box<(dyn xilem_web::Message + 'static)>>>
              <Adapt<ParentState, ParentAction, ChildState, ChildAction, Context, V, Message, F> as View<ParentState, ParentAction, Context, Message>>
              <Address<State, Action> as View<State, Action, ViewCtx, Box<(dyn xilem_web::Message + 'static)>>>
            and 314 others
    = note: required for `Fork<(Div<_, _>, Div<_, _>), ...>` to implement `View<State, (), ViewCtx, Box<(dyn xilem_web::Message + 'static)>>`
    = note: required for `Fork<(Div<_, _>, Div<_, _>), ...>` to implement `ViewSequence<State, (), ViewCtx, Pod<DynNode, Box<(dyn Any + 'static)>>, Box<(dyn xilem_web::Message + 'static)>>`
    = note: required for `Fork<(Div<_, _>, Div<_, _>), ...>` to implement `DomFragment<State>`
    = note: the full name for the type has been written to '/Users/rodarmor/src/gossamer/target/debug/deps/library_viewer-fd8c1bd6c102b4f4.long-type-13969890497949773298.txt'
    = note: consider using `--verbose` to print the full type name to the console
    = note: the full name for the type has been written to '/Users/rodarmor/src/gossamer/target/debug/deps/library_viewer-fd8c1bd6c102b4f4.long-type-13969890497949773298.txt'
    = note: consider using `--verbose` to print the full type name to the console
    = note: the full name for the type has been written to '/Users/rodarmor/src/gossamer/target/debug/deps/library_viewer-fd8c1bd6c102b4f4.long-type-13969890497949773298.txt'
    = note: consider using `--verbose` to print the full type name to the console

[Finished running. Exit status: 101]

@DJMcNab
Copy link
Member

DJMcNab commented Aug 6, 2024

(div(()), fork(div(()), ...)) should work, though, right?

@casey
Copy link
Contributor Author

casey commented Aug 10, 2024

Yup, that does work, although it's a little weird for my use case, since I want foo, in fork(foo, bar) to be the top-level view, and bar to contain nothing but alongside views. So (a, fork(b, alongside)) would require moving something from foo into b, which doesn't make a whole lot of sense.

@Philipp-M
Copy link
Contributor

Yeah I see, intuitively it would make sense, but I think it's not easily possible to do this with the same function. We already have to fight for ambiguity quite a lot (e.g. View <-> ViewSequence). I don't think it can return either an impl ViewSequence or an impl View (at least from a typing perspective, since an impl View is also an impl ViewSequence).

We could add fork_seq though, which would allow sequences as first parameter...

@casey
Copy link
Contributor Author

casey commented Aug 11, 2024

Possibly more useful would be a single-argument function which could be used to host any number of NoElement views, so they could be inserted anywhere without having to restructure the existing code:

div(
  h1("Foo"),
  ignore(memoized_await(…)),
)

I think that probably, the ideal state would be if users could just stick things like memoized_await into their views, and have just not produce any elements, but if that isn't possible for type-system reasons, then something which can just host NoElement views might be a good idea. Although maybe you run into the same type-system problems which prevent sticking memoized_await into views in the first place.

@Philipp-M
Copy link
Contributor

Possibly more useful would be a single-argument function which could be used to host any number of NoElement views, so they could be inserted anywhere without having to restructure the existing code:

#608 should make this possible.

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 a pull request may close this issue.

3 participants