leptos-rs / leptos

Build fast web applications with Rust.
https://leptos.dev
MIT License
16.78k stars 672 forks source link

If it's not possible to clear all associated event listeners when a component is unmounted, at least support uninterrupted global event listeners! For imported third-party js libraries, such support is very important. #1555

Closed lhjok closed 1 year ago

lhjok commented 1 year ago

If it's not possible to clear all associated event listeners when a component is unmounted, at least support uninterrupted global event listeners! For imported third-party js libraries, such support is very important.

gbj commented 1 year ago

I don't understand what this means and it doesn't follow the issue template. Could you please provide more details about the issue you are facing? Thank you.

(If you mean "support removing global event listeners" then this is planned for 0.5.0, it is just a breaking change to the window_event_listener function so I'm combining it with other breaking changes for that release. See docs for window_event_listener in 0.5.0-beta.

lhjok commented 1 year ago

I mean, if you initialize the event listener in the root component, then it should apply to all its child components, without breaking the event listener connection when switching components.

gbj commented 1 year ago

I'm sorry, I do not understand. Could you provide a code example to show what you mean?

lhjok commented 1 year ago
#![allow(non_snake_case)]
mod views;
use leptos::*;
use leptos_router::*;
use views::{ Login, Home, Admin, Error };

use wasm_bindgen::prelude::*;

#[wasm_bindgen(module=
"/static/scripts/elements.js")]
extern {
    #[wasm_bindgen(js_name= initDropdown)]
    fn init_dropdown(id: &str);
}

fn main() {
    console_error_panic_hook::set_once();
    mount_to_body(|cx| view! { cx, <App/> })
}

#[component]
fn App(cx: Scope) -> impl IntoView {
    view! { cx,
        <Router>
            <Routes>
                <Route path="/" view=Home/>
                <Route path="/login" view=Login/>
                <Route path="/admin" view=Admin/>
                <Route path="/404" view=Error/>
            </Routes>
            { request_animation_frame(move || {
                init_dropdown("[data-te-dropdown-toggle-ref]");
            }) }
        </Router>
    }
}
lhjok commented 1 year ago

When you route to Home and Login or other components, it will interrupt the event listeners initialized in the App component.

lhjok commented 1 year ago

If you route to the login component, and return to the app component, and then enter the login component again, you will find that the event listener has no effect.

lhjok commented 1 year ago

This may be because the DOM object recreated after the DOM object is deleted is not the same object as the original object, so the originally bound event listener is interrupted.

gbj commented 1 year ago

I don't know what the JavaScript you are calling does, but it sounds like you are misunderstanding it. My assumption would be that this

init_dropdown("[data-te-dropdown-toggle-ref]");

finds every element with the attribute data-te-dropdown-toggle-ref that is currently on the page, and imperatively adds an event listener to each of those elements. This does not, I assume, create a long-living process like a DOM MutationObserver that listens for any new elements in the DOM, checks whether they have the data-te-dropdown-toggle-ref attribute, and adds the dropdown event listener.

You are only calling the function once here, so I would not expect it to add the same event listeners if you navigate away from the page (using client-side navigation), removing all the dropdowns, and then navigate back to it.

This isn't the framework "interrupting" event listeners; you're just only adding them once, to a snapshot of the page, so when the page changes in the future, they aren't added to elements you create in the future unless you call the function again, selecting the new elements.

lhjok commented 1 year ago

If called again, there will be a problem of repeatedly registering event listeners.

lhjok commented 1 year ago

Many third-party js libraries do not support the method of manually calling the detached event listener, which makes me unable to continue unless the third-party js library is not used.

lhjok commented 1 year ago

If there is a global long-term process listening for any new elements in the DOM, it will be much easier for developers, and more third-party libraries can be used.

gbj commented 1 year ago

Did we already have this entire conversation on Discord a couple weeks ago?

Integrating a JS library that only support imperatively adding event listeners, and does not support removing them, with a component-based/declarative framework that uses client-side routing will always be hard.

Are you allowed to tell it which specific elements you want to add the dropdown behavior to? Because then you can do something like

#[component]
fn Dropdown(cx: Scope) -> impl IntoView {
  let dropdown_ref = create_node_ref::<html::Div>(cx);
  create_effect(cx, move |_| {
    if let Some(el) = dropdown_ref.get() { 
      request_animation_frame(move || {
        init_dropdown(el);
      });
    } 
  });
  view! { cx,
    <div node_ref=dropdown_ref>
      // ...
    </div>
  }
}

If you're not allowed to pass it a DOM element, then can you give it an ID selector?

#[component]
fn Dropdown(cx: Scope) -> impl IntoView {
  let id = Uuid::new_v4();
  create_effect(cx, move |_| {
    request_animation_frame(move || {
      init_dropdown(format!("#{id}"));
    });
  });
  view! { cx,
    <div id=id.to_string()>
      // ...
    </div>
  }
}
gbj commented 1 year ago

If there is a global long-term process listening for any new elements in the DOM, it will be much easier for developers, and more third-party libraries can be used.

This is outside the scope of this (or any?) framework. The browser provides native support for it with MutationObserver, which you can use. Or you can use a JS library with an API that allows you to do what you need to do.

lhjok commented 1 year ago

I also found many such UI components, and they did not provide me with an API to separate event listeners. I submitted a request to their project, and they responded that they were not interested in supporting my idea.

lhjok commented 1 year ago

If they provided it, I could manually detach it myself, but it doesn't seem ideal.

gbj commented 1 year ago

It seems to me that removing the event listeners it not really the issue: only adding the event listeners to the newly-created elements, rather than to every relevant element on the page, would be sufficient.

Would one of the two options I posted above be allowed?

lhjok commented 1 year ago

If MutationObserver is initialized with a callback function, the problem of repeated registration of event listeners will still occur. The properties defined in the third-party library are fixed and I cannot modify them.

gbj commented 1 year ago

The library does not allow you to add the behavior for a single element, by ID or by reference to that element?

lhjok commented 1 year ago

We cannot require other third-party libraries to be designed according to our requirements. This is unrealistic. I have not found an effective way to proceed.

lhjok commented 1 year ago

The library does not allow you to add the behavior for a single element, by ID or by reference to that element?

Yes, maybe they use their own defined attributes in other places, and changing them to IDs by themselves will not achieve the expected effect.

gbj commented 1 year ago

I guess I just don't understand how a library like that would be integrated with any frontend framework of the last ~10 years. Do they have a React integration or something so we can look at how that's done? Libraries that are designed for a multi-page app with full page refresh on every navigation do just need some adapting to work with single-page apps that use client-side navigation, sorry.

lhjok commented 1 year ago

Even if you only add a single element without deleting the event listener, there will still be the problem of repeatedly registering the listener.

lhjok commented 1 year ago

They support React and vue

lhjok commented 1 year ago

https://github.com/mdbootstrap/Tailwind-Elements

gbj commented 1 year ago

Looking through some old issues having to do with NextJS, I came across their NextJS integration tutorial. It looks like they do, in fact, support initializing their components for a single element by ID. The example below should be fairly straightforward to adapt to what I suggested in the "can you give it an ID selector?" example above

const MyComponent = () => {
  useEffect(() => {
    const init = async () => {
      const { Datepicker, Input, initTE } = await import("tw-elements");

      const myInput = new Input(document.getElementById("myDatepicker"));
      const options = {
        format: "dd-mm-yyyy",
      };
      const myDatepicker = new Datepicker(
        document.getElementById("myDatepicker"),
        options
      );
    }
    init();
  }, []);

  return (
    <div
      className="relative mb-3"
      id="myDatepicker"
    >
    // ...

See here: https://tailwind-elements.com/docs/standard/integrations/next-integration/

Editing to add: Looking at their API docs for random components like Dropdown it looks like they show "Via Data Attributes" and "Via JavaScript" options for each. You want to use the "Via JavaScript" and create a binding for each of the component constructors you need to call, passing it the element by using a NodeRef or .getElementById():

const exampleEl = document.querySelector('example');
const myButton = new Button(exampleEl);

I don't think using this API requires additional framework support.

lhjok commented 1 year ago

This is initialization in the component. When the component is uninstalled, the event listener cannot be automatically cleared. Next time you come back, you will re-register it.

lhjok commented 1 year ago

There is also a problem of repeatedly registering listeners. This library does not provide an API for separating listeners.

gbj commented 1 year ago

But when the component is unmounted, that element is removed from the DOM... You will re-register it, but on a new element. Leptos components do not run multiple times like React elements.

gbj commented 1 year ago

if you can provide an example, using the "Via JS" API with a constructor that takes an element like what is above, that causes a problem, I'd be happy to look at it. Otherwise, I don't think continuing to go back and forth like this is productive.

lhjok commented 1 year ago

I did encounter the problem of duplicate registration, but I used the class selector before, I will try to use the id selector.

lhjok commented 1 year ago

Thank you very much for your patience

lhjok commented 1 year ago

But when the component is unmounted, that element is removed from the DOM... You will re-register it, but on a new element. Leptos components do not run multiple times like React elements.

Am I writing something wrong? Every time you enter the login component, an event listener will still be registered. Here is the code

gbj commented 1 year ago

What is the issue you are experiencing with the code you have written? When I compile and run it, it seems to be fine.

The only issue I could see is that this line

let dropdowns = init_dropdown("[data-te-dropdown-toggle-ref]").unwrap();

will cause it to add an event listener to every element on the page that has a data-te-dropdown-toggle-ref attribute, whether it's in the Login component or not, because that's exactly what you're telling it to do.

This is, again, different from the example I gave above, in which you would create a Dropdown component of your own which would add the listener for only the single element you are creating.

#[component]
fn Dropdown(cx: Scope) -> impl IntoView {
  let id = Uuid::new_v4();
  create_effect(cx, move |_| {
    request_animation_frame(move || {
      init_dropdown(format!("#{id}"));
    });
  });
  view! { cx,
    <button id=id.to_string()>
      // ...
    </button>
  }
}
lhjok commented 1 year ago
let _name = Input::new(&document().get_element_by_id("inputName").unwrap());
let _pass = Input::new(&document().get_element_by_id("inputPass").unwrap());
lhjok commented 1 year ago

Every time you enter the component, Input will register an event listener. You can check the browser developer tools for the event listener inside. Inside you can see every new event added.

lhjok commented 1 year ago
let dropdowns = init_dropdown("[data-te-dropdown-toggle-ref]").unwrap();

Since this is an event listener created by itself, the event listener will be automatically cleared when the component is unloaded.

lhjok commented 1 year ago
let _name = Input::new(&document().get_element_by_id("inputName").unwrap());
let _pass = Input::new(&document().get_element_by_id("inputPass").unwrap());

The above Input is initialized by a third-party event listener, so after unloading the component, the event listener is still retained.

lhjok commented 1 year ago

截图 2023-08-18 02-44-37

gbj commented 1 year ago

Oh, wow. So they are delegating all their events to document and provide no way of removing them. I've tested it and even the dispose() method they provide doesn't remove the event listeners they add.

If they added event listeners to the actual elements you pass them, those event listeners would be cleaned up when the element was removed. But instead, they add all their event listeners to document and they don't clean up after themselves.

It's not even possible to remove an event listener without having access to the original callback/function in order to remove it, so I don't think there's any way for you or me to do anything about this. Unless they intend for these to be cleaned up some other way, this is basically a memory leak built into the API design... as far as I can tell there's no way for any of those events to be removed so if you initialize multiple components they just keep piling up.

I am not able to spend any more time reading through their docs and source code to try to figure this out, unfortunately.

lhjok commented 1 year ago

If it is a componentized design, it is obviously unreasonable to do so. I just don’t know how they designed it on React. It is reasonable to encounter the same problem. The single-page application does not refresh the entire page. In this way, it is similar to a global event listener. How do they listen across components? of?

gbj commented 1 year ago

It looks like their examples for e.g., React/NextJS integration do exactly what I did. I won't be able to explore it more but I suspect it just piles up adding additional document listeners.

There's nothing Leptos (or any other framework) can do to remove event listeners that a third-party library is adding to the document, if that library doesn't provide any way to remove them. I'm going to close this issue, as I don't think there's anything I can do that can solve this problem for you.

If you're just looking for a component library to use, by the way, and you don't require this particular component library, you might want to take a look at Leptonic, which is building a native Leptos UI component library.

lhjok commented 1 year ago

Thank you very much for your help, at the moment I'm also in the learning stage, no project in the works yet, the reason I'm focusing on this ui library is because I want to explore how to port it to the Leptos project, this kind of component based on the native DOM structure, the style can be customized to a higher degree, and they have a large number of components, if I can wrap it directly into the Leptos UI, it will save a great amount of work that way.