Change vdom bootstrapping to occur before the initial orders are processed.

[AlterionX]

Changes:

• Change execution of the initial call to process_cmd_and_msg_queue to after the main_el_vdom is initialized. This allows for the msg processing to not cause issues if it attempts to force a render or something else similar. Forcing a render on init used to cause Seed to panic.
• Allow Seed to take over the children of the mount. This can be useful for SSR. Built-in SSR for Seed is my goal for now. This is partially related to Question/Feature suggestion: Server side rendering #232, which is also related to Proposal: Hydrate #223.
• Warns of possibly questionable behavior when attempting to accomplish the above maneuver as it is now (see the doc comment for takeover_mount in AppBuilder). Due to this behavior and backwards compatibility, it is currently an opt in functionality.
• Adds a bunch of utility methods to the Node enum.
• Adds a function to recursively remove all workspace web_sys::Nodes.

Parts that need additional consideration (incomplete)

• Unsure if the listeners in the bootstrap should be set up -- not familiar with what they do. But it seems to work for now.
• Unsure of any other implications of moving the initial order processing to after the initialization of the main_el_vdom.
• Not sure if any of the utility methods added to the Node enum are actually of use.
• Might want to fully address the "recreation" behavior of bootstrap_vdom before this gets merged.

[MartinKavik]

@AlterionX Your changes list looks promising! I'll try to do code review and think about incomplete list in a few days.

[MartinKavik]

@AlterionX

• It's possible that this PR also resolves Root element panics at 'Can't find old handler' #198
• I cloned your master (aka this PR) and when I start an example (e.g. cargo make start counter) there is no errors but also no content (blank page). So I assume you are working on it and I don't want to change code under your hands => let me know when you need some help / it's ready for some (partial) review. Thanks.

[AlterionX]

Oops, I forgot to force a render at the start. I was primarily doing testing with my own app, so I'll try to fix that now.

[MartinKavik]

So I've done a first round of code review:

1. We should try to implement traits instead of writing new custom methods. E.g.:

   • src/dom_types.rs - Text::get_text(..) can be replaced with impl Display for Text
   • src/websys_bridge - el_from_ws_element(..) => impl From<&web_sys::Element> for El<Ms>

2. Match function name with it's body.

   • E.g. method strip_ws_nodes:
     • Why the body is self.node_ws.take(); and not self.node_ws = None; => should we rename it to take_ws_node(s)?
     • Why the name is ending with s? Text has only one node_ws. If it should work the same for more entities (e.g. El, Svg, Text), we should abstract it to a trait or implement it directly on Node (if it makes sense).
     • It's a little bit surprising that strip_ws_nodes deletes also children nodes for El - at least a comment would be nice.

3. Probably unnecessary helpers like map_el_in_place, map_or_else_ref, map_in_place, strip_ws_nodes and process_cmd_and_msg_queue_with_forced_render.

   • Is it code really more readable and maintanable with them?
   • It's possible that there will be Node variant Svg. We'll have to update map_in_place, map_or_else_ref, many methods like El:add_attr(..), write method map_svg_in_place, etc. - is it a good trade-off?

4. Don't be afraid to extract some parts of code into standalone modules (files) - see /src/dom_types/values.rs. E.g.:

   • el_from_ws_element + node_from_ws from src/websys_bridge.rs
   • AppBuilder + its implementations / helpers from vdom.rs

5. Don't be afraid to extract some parts of big functions.

   • E.g. el_from_ws_element - block with comment Populate attributes and the last block with the loop. (Comments like Populate attributes often signal that we should extract code below to functions with the similar name).

6. More flexible / simpler public API for AppBuilder::takeover_mount(..).

   • I think we don't need parameter should_takeover - we just don't call that method at all.
   • What if we need another rendering mode (e.g. we'll find out that we need different modes for prerendering and SSR)? I'll basically don't like bools because they aren't expressive enough and you can't add more values into them in the future.
     • I suggest to introduce a new enum like MountType = Takeover | Append.
     • And .takeover_mount() replace with .mount_type(MountType::TakeOver).
     • Another option is to reuse .mount but it will be breaking change and you would have to define explicitly mount point even if you want to use the default one.
     • cc @David-OConnor

---

I didn't dive deeper into code logic (e.g. why Unrecognized tags are also converted into spans, so take note.). I'll do it in the next round.

@AlterionX Well, it looks like I don't like the code but it's not true, I think it's very good and resolves many pains - my points are opinionated and the PR has many changes so it's a good opportunity to refactor it a little bit during process.

[AlterionX]

All good points. I didn't even really think about it.

Regarding get_text, that was simply a method moved from elsewhere. Most of these changes simply followed what existed before.

Not sure if I should here, but I can try scanning over the entire library to see if there are some other things that can be pulled out?

[MartinKavik]

Regarding get_text, that was simply a method moved from elsewhere.

There are parts which don't make sense now or aren't super clean - don't be afraid to change it / create issue / write todo. (Just like every other project basically.)

Try scanning over the entire library to see if there are some other things that can be pulled out?

I recommend to focus mostly on things which you are changing or are somehow related - just make it not too big for review and resolving conflicts with other PRs :)

[AlterionX]

Got a question: is there a reason for the functions being put into the App struct function pointers instead of Fn trait structs? If not, I'm thinking of changing that in a different PR.

[MartinKavik]

is there a reason for the functions being put into the model being function pointers

I'm not sure which functions do you mean (?). But in general - we try to write every public API with FnOnce for maximum flexibility (#169 (comment)).

[AlterionX]

I'm referring to SinkFn, UpdateFn, etc. Also, I think I figured out why they're that way, so nevermind.

[MartinKavik]

I think that there aren't any specific reasons. Why are you asking?

[AlterionX]

I think if you make it generic, the compiler will complain that it can't figure out what the functions are actually supposed to be if you don't use, say, sink, when building the AppBuilder. This would then force you to specify the full type of AppBuilder, which is long as heck, making it extremely not ergonomic for the base case where you only use build. This is mostly just speculation, since I haven't actually tried it out, but still.

[MartinKavik]

You can write them as Fn, I just don't see any real-world advantage now. Create PR if you find it useful.
E.g. SinkFn can rewritten to:

type SinkFn<Ms, Mdl, ElC, GMs> = Box<dyn Fn(GMs, &mut Mdl, &mut OrdersContainer<Ms, Mdl, ElC, GMs>)>;

(and compiled with one small change in code).

[AlterionX]

Hrm, not exactly what I meant.

I was wondering why SinkFn wasn't something like

trait SinkFn<Ms, Mdl, ElC, GMs>: FnMut(GMs, &mut Mdl, &mut OrdersContainer<Ms, Mdl, ElC, GMs>) {}
impl<Ms, Mdl, ElC, GMs, F> SinkFn for F
where
    F: FnMut(GMs, &mut Mdl, &mut OrdersContainer<Ms, Mdl, ElC, GMs>) {}

[AlterionX]

@MartinKavik @David-OConnor I'm starting to get half a mind to move the Init from build to run or something similar to isolate all the side effects inside of run instead of distributing the stateful effects across finish and run.

I think that this is better since we do this weird dance around initial_orders right now where we don't quite have the correct state to do what we have half the state we want inside Builder and half the state inside the App.

So we would have

App::build(update, view).finish().run_with_init(|_, _| {
    // Do things with url + orders.
    // `Init::default` is a wrapper for `Init::new(Model::default())`.
    Init::default()
})

or

App::build(update, view).finish().run() // Calls `Init::default` by default.

It's a bit of a departure from what we currently have, but I think that it makes the implementation a little less weird and the usage clearer about when exactly the InitFn happens (since it seems to run half in finish and half in run right now).

Personally, I also think that mount should also be inside Init since we might want to mount it to a different place depending on the initial route, but we seem to keep that around forever, so I changed my mind. Point is, there are parts of AppCfg that are not immutable, and I would like to change that.

The downsides, I think, are also fairly obvious in that it's a breaking change, and so on. Also, the model is now provided in the run method instead of the build method, which is also... unique, so to speak.

I haven't seen if the types work out yet, but it seems reasonable to me.

[AlterionX]

Whoops, didn't mean to do that.

[AlterionX]

@MartinKavik I think it's ready for another go. Could you use the GitHub review stuff if you're going to make specific comments about specific function, please? It's easier to see what's being talked about that way. Also got a few comments/questions, by the way. I think I made/implemented the other suggestions.

Why the body is self.node_ws.take(); and not self.node_ws = None; => should we rename it to take_ws_node(s)?

self.node_ws.take(); and self.node_ws = None; accomplish the same goal, so I don't think it matters. The end result is still the same. If we want to get really banal about it, we could say that take leaves the enum in place, while = None overwrites the entire enum, but it's not that important since the compiler probably optimizes it like crazy anyways.

I think we don't need parameter should_takeover - we just don't call that method at all.

Not entirely sure what method this is talking about. should_takeover is a field, and is passed on to the app, which uses it later. I did change it into an enum and make it part of Init, however.

[AlterionX]

Closing to trigger another travis build -- I think it might be a Mac specific error, but it seemed to work just fine on linux.

[David-OConnor]

@MartinKavik : Any objections to merging this?

[MartinKavik]

Can we rename it to mount_type and MountType so users don't have to relearn name once we change API to this one: #250 (comment)?

[MartinKavik]

@David-OConnor I wrote some comments. It seems that the only public API change is mount_type (bootstrap_behavior) in Init (?). If examples work and once the comments are resolved, I'm ok with merge.

[AlterionX]

I'm trying to pull out a few things from this PR into smaller PRs. Will fix the names and return types soon.

[AlterionX]

Closing since changes are implemented in #269, #270, #271, #274, #276.