Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

New EventListener patching, refactors, ElRef, nodes!, etc. #330

Merged
merged 16 commits into from
Jan 14, 2020

Conversation

MartinKavik
Copy link
Member

It's a big PR, but each commit has been checked with cargo make verify so you can review them one by one. I recommend to read CHANGELOG.md as the first step.

I think the changelog is sufficient as the list of changes for this PR, I would rather write about reasons why or how I've made these changes.

  • Removed old update & trigger_update_ev, lifecycle hooks, internal input listeners - they were incomplete or their behavior was surprising or better alternatives exist. Removing was the first step before other refactors because those features made codebase more complicated.

  • Rewritten event listener patching. The biggest problem was constant reattaching of all listeners - it was slow and error-prone because it often kills fired event in fly.
    How it works now:

    • Each El contains EventHandlerManager. (There is also a "special" EventHandlerManager for window.)
    • Each EventHandlerManager contains and manages EventHandlers and Listeners for the given El. EventHandler is a user's handler - it's created in the app code and returns Msg. Listener is the Seed internal representation of a native DOM EventListener.
    • EventHandlers are grouped by their Ev (a.k.a event / trigger) and each group may have one Listener. This Listener has the same Ev like the EventHandlers and it is created during VDOM patching or it can be moved from the old element version (i.e. reused).
    • Listener calls all EventHandlers from the group once the associated event Ev is fired.
    • Listener automatically detach associated DOM EventListener on drop.
    • Listeners should be reused as much as possible to prevent lost events and performance issues.
    • Listeners are connected to the EventHandlers through Listener field portal. Portal allows to modify EventHandlers group and also change referenced EventHandler group even when the Listener's callback has been already sent to the JS world and registered as the DOM EventListener.
  • All examples refactored. I've added a simpler counter example (the old one is renamed to counter_advanced) because we need something for the beginners who see a Seed app (and maybe Rust) for the first time. And I've removed example server_interaction_detailed because I have no idea what is it, what users can learn from it and it doesn't work (?) + looks very incomplete.

  • Added el_ref + ElRef - users can use native DOM elements safely because ElReg::get checks if the referenced element exists, is in the DOM now and has the right type. And users don't have to use any error-prone selectors. See examples (canvas, user_media or todomvc).
    I'm not very happy with the implementation, but I've chosen the less invasive one and I want to refactor patch.rs, virtual_dom_bridge.rs and related things in the future - there are too many "bad things" like expects and side-effects scattered in this code, so it's very error-prone.

  • Added macro nodes!. I had to often write something like:

vec![
    vec![ view_one_node() ],
    view_multiple_nodes(),
].into_iter().flatten().collect()

It can be rewritten to:

nodes![
    view_one_node(),
    view_multiple_nodes(),
]

Or nodes![] can be the alternative to vec![] as the equivalent to empty![] for multiple nodes.

  • Ev, Tag, St and At have consistent API now and can be created by X:from(Into<Cow<'static, str>). I think the most custom instances are created with the 'static str (e.g. At::from("my-attribute")) so it should improve performance a bit, because, for instance, Ev is often cloned in the Seed codebase because of event-handling and patching requirements.
    However this change will be almost the internal one in the future, because we will have a different APIs for working with those entities - for instance new API for attributes is ready for implementation.

  • raw_ev is deprecated in favor of ev (i.e. renamed). Using simple_ev is problematic because it requires Clone implementation for Ms and you had to rewrite it into raw_ev when you want to call e.g. prevent_default. It makes app code a little bit less consistent and some Msg data can't be cloned so you have to wrap them with something like Rc. So I'd rather use raw_ev everywhere instead of simple_ev. But raw_ev is unnecessary verbose + all events in handlers are "raw" (they have only different type) so the name isn't also very accurate.

@David-OConnor
Copy link
Member

I need to dive in more thoroughly, but from your summary and a skim of the code, I agree on all points.

Highlights:

  • nodes is nice, and long overdue as a way to avoid type conflicts between Node and Vec<Node>.; it's consistent with the flexible view syntax.
  • Agree on removing server_interaction_detailed; it was a mess.
  • The quickstart repo's lib.rs served previously a minimal counter example. The new counter appears even more minimal, so I think it's worth it for that, or when people use the webpack quickstart. The counter_advanced (prev counter) is to show a range of common features.
  • Love the event patching refactor; that was a problem since the beginning. Unlike other parts of the VDOM, that was hard to deal with due to the closures.
  • I like the breaking simple_ev/raw_ev change; it's more consistent between the two that way.

@MartinKavik MartinKavik force-pushed the feat/virtual_dom_update branch from 58f09fd to 663338e Compare January 12, 2020 16:38
@David-OConnor
Copy link
Member

Have you noticed a performance improvement from the listener change?

@MartinKavik
Copy link
Member Author

MartinKavik commented Jan 13, 2020

I think that examples in the repo are too small to notice slowdown due to listener attachments and Rust is too fast to notice slowdown due to DOM traversing by the naked eye. Maybe once benchmark is set up (#324) we can test it with adding listeners to window and test rows.

@rebo
Copy link
Collaborator

rebo commented Jan 13, 2020

Definitely like nodes! as a standin for vec! Node or Node. It will simplify a lot!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants