Delete builder module #457

Ben-PH · 2020-05-14T16:44:57Z

Relates to #453

Concerns about #277 outlined in comments, and will need to be resolved before pulling in.

src/app.rs

Ben-PH · 2020-05-14T16:54:26Z

src/app.rs

-    /// Bootstrap the dom with the vdom by taking over all children of the mount point and
-    /// replacing them with the vdom if requested. Will otherwise ignore the original children of
-    /// the mount point.
-    fn bootstrap_vdom(&self, mount_type: MountType) -> El<Ms> {


This is directly relevant to issue #277

From the code that I was working with, mount_type was only ever passed in with ::Takeover once App::new() was made private. Given that this case only ever returns the result of El::empty(Tag::Placeholder), I opted to replace calls to this function with that.

src/app.rs

src/app/cfg.rs

src/app/orders/container.rs

MartinKavik · 2020-05-14T18:47:08Z

src/app.rs

-            Some(app_init_cfg),
-            base_path,
+        let root_element = root_element.get_element().expect("get root element");
+        let app = Self::new(update, view, root_element, base_path.clone());


Please use Rc::clone where possible (see the old code in this diff).

https://doc.rust-lang.org/book/ch15-04-rc.html#using-rct-to-share-data:

We could have called a.clone() rather than Rc::clone(&a), but Rust’s convention is to use Rc::clone in this case. The implementation of Rc::clone doesn’t make a deep copy of all the data like most types’ implementations of clone do. The call to Rc::clone only increments the reference count, which doesn’t take much time. Deep copies of data can take a lot of time. By using Rc::clone for reference counting, we can visually distinguish between the deep-copy kinds of clones and the kinds of clones that increase the reference count. When looking for performance problems in the code, we only need to consider the deep-copy clones and can disregard calls to Rc::clone.

base_path is already an Rc. Are you talking about the app itself?

That sould work for the app, but I think it might be digging ourselves into a hole. At the moment, making a new app is dependent on having a model, but once you have that app, it needs to take ownership of the model. Rc can help us get around it, but it feels a bit hacky - kinda like a circular dependency. I suspect that there's a better way to do it.

Outside of this piece of code, the nature of the data doesn't seem to create any need of a ref-count. I think part of cleaning up start should include clearing away deep-copies and unnecisary Rc wrapping.

How about we pull this into a development branch? That way we can have a go at not having deep-copy AND not wrapping it in Rc. If it doesn't work out, we can fix it up before bringing it into production.

base_path is already an Rc. Are you talking about the app itself?

Please read my comment and copy-pasted explanation again :)

Rc::clone(&base_path) vs base_path.clone() => just replace .clone with Rc::clone.

MartinKavik · 2020-05-14T18:52:17Z

src/app.rs

            cfg: Rc::new(AppCfg {
                document,
                mount_point,
                update,
-                sink,
+                sink: None,


Just note to not forget to remove sink, window_events and routes.

So I sorted that out. sink and window_events i don't recall making too much difference (my memory could be wrong there, so take with a grain of salt). Culling routes, though, cascaded into a few other code changes that come with broader consequences.

MartinKavik · 2020-05-14T18:55:04Z

src/app.rs

    rc::Rc,
 };
-use types::{RoutesFn, SinkFn, UpdateFn, ViewFn, WindowEventsFn};
+use types::{UpdateFn, ViewFn};


We should consider to refactor / remove types module.

next PR, or add it to this one?

I don't know how hard it would be - your choice.

I'll leave it to another PR. Taking out window_events might reveal some bugs, and I we might be biting off more than we can chew already.

I'm happy for this PR to be put onto a dev-branch, then bring in a bunch of tests for the examples (and real-world app, I think) to make sure everything's okay. Then we can look at types. How does that sound?

I think this PR should be merged into master before we'll create a new one. You don't want to resolve complicated conflicts once somebody push bigger changes to master and forgot context while you are working on other parts in a new PR.

Hmmm, I can only disagree from a place of relative lack of experience. I can see the logic in it.

src/app.rs

Ben-PH · 2020-05-15T01:11:30Z

src/app.rs

@@ -299,18 +267,6 @@ impl<Ms, Mdl, INodes: IntoNodes<Ms> + 'static, GMs: 'static> App<Ms, Mdl, INodes
        }
    }

-    pub fn patch_window_event_handlers(&self) {


Remember: we drop window_events, making this redundant.

Try cargo make start subscribe => http://localhost:8000/ => resize window => check that Window size: ww x hh has been changed. If it works, we don't need this code anymore.

Ben-PH · 2020-05-15T03:00:27Z

src/app.rs

@@ -287,8 +255,8 @@ impl<Ms, Mdl, INodes: IntoNodes<Ms> + 'static, GMs: 'static> App<Ms, Mdl, INodes
                    let mut new_effects = self.process_queue_message(msg);
                    queue.append(&mut new_effects);
                }
-                Effect::GMsg(g_msg) => {


I don't understand the broader system well enough to recognise what this change implies. A grep of Effect::GMsg came up with 4 hits:

This one

Pushing a message onto the global message queue here

10 lines up pushing onto front as part of the sink function, which I understand we plan to remove.

does nothing

So, in terms of utility, summarizing the use of global message

We are discarding it as an unused variable

We are storing the value into a queue.

Used by objects implementing the Orders trait.

is effectively a no-op as far as GM is concerned

Interesting...

Aside from running self.data.msg_listeners callbacks, and running the update function, the code is the same, but not if we consider OrdersContainer construction because it always gives ShouldRender::Render. This means that all it does is call self.schedule_render() and return an empty queue.

Possible issues?

Well, maybe. I can't see how the system uses the value that ::GMsg wraps, and I feel a bit blind as to the motivations to put in sinkin the first place. What are we throwing away here?

sink, Effect::GMsg, <...GMs..>, send_g_gms, perform_g_cmdand perform_g_cmd_with_handle should be removed.

Global messages work the same like regular messages (Msg / Ms), but the whole app use only one type. This mechanism of inter-component communication has been replaced by orders.subscribe / notify. Everything GMsg-related will be deleted.

Ben-PH · 2020-05-22T07:48:08Z

Closing, as breakages have come up.

Ben-PH commented May 14, 2020

View reviewed changes

src/app.rs Outdated Show resolved Hide resolved

Ben-PH commented May 14, 2020

View reviewed changes

src/app.rs Show resolved Hide resolved

Ben-PH commented May 14, 2020

View reviewed changes

src/app/cfg.rs Show resolved Hide resolved

Ben-PH commented May 14, 2020

View reviewed changes

src/app/orders/container.rs Show resolved Hide resolved

Ben-PH force-pushed the builder_clear branch from e34be37 to d7ee11f Compare May 14, 2020 17:52

MartinKavik reviewed May 14, 2020

View reviewed changes

src/app.rs Show resolved Hide resolved

Ben-PH force-pushed the builder_clear branch 2 times, most recently from 853af1e to 30614b1 Compare May 15, 2020 00:07

Ben-PH commented May 15, 2020

View reviewed changes

Delete builder module

e5eb430

Ben-PH force-pushed the builder_clear branch from 30614b1 to e5eb430 Compare May 15, 2020 03:10

Ben-PH closed this May 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete builder module #457

Delete builder module #457

Ben-PH commented May 14, 2020

Ben-PH May 14, 2020

MartinKavik May 14, 2020

Ben-PH May 14, 2020

MartinKavik May 15, 2020

MartinKavik May 14, 2020 •

edited

Loading

Ben-PH May 15, 2020

MartinKavik May 14, 2020

Ben-PH May 14, 2020

MartinKavik May 14, 2020

Ben-PH May 15, 2020

MartinKavik May 15, 2020

Ben-PH May 15, 2020

Ben-PH May 15, 2020

MartinKavik May 15, 2020 •

edited

Loading

Ben-PH May 15, 2020 •

edited

Loading

MartinKavik May 15, 2020

Ben-PH commented May 22, 2020

Delete builder module #457

Delete builder module #457

Conversation

Ben-PH commented May 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinKavik May 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinKavik May 15, 2020 • edited Loading

Choose a reason for hiding this comment

Ben-PH May 15, 2020 • edited Loading

Choose a reason for hiding this comment

Interesting...

Possible issues?

Choose a reason for hiding this comment

Ben-PH commented May 22, 2020

MartinKavik May 14, 2020 •

edited

Loading

MartinKavik May 15, 2020 •

edited

Loading

Ben-PH May 15, 2020 •

edited

Loading