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

fix(event): clear residual js listeners (#8916 ) #8930

Merged

Conversation

canxin121
Copy link
Contributor

fix clear residual listeners, close #8916

I noticed that in v1, js_listeners were stored on the frontend, so refreshing the page cleans up the original js_listeners (note that this doesn't mean that useEffect cleans up the side-effects, as it doesn't have a way to clean up the side-effects before refreshing the page).

However, in v2, js_listeners are stored not only in the front end, but also in the back end, and the number of events triggered is determined by the js_listeners that stored in the backend, which results in a pileup of js_listeners that can't be cleaned up when the page is refreshed.

I added a new js command unlisten_all and invoke it at the end of core.js to solve this problem. unlisten_all only cancels the js_listeners at the frontend and backend of the current window.

This fix works fine on my machine, in both single-window and multi-window scenarios.

@canxin121 canxin121 requested a review from a team as a code owner February 21, 2024 11:49
@canxin121
Copy link
Contributor Author

A fixed demo

fixed.mp4

The origin error demo can be seen at #8916

@canxin121 canxin121 changed the title fix(event): clear residual listeners (#8916 ) fix(event): clear residual js listeners (#8916 ) Feb 21, 2024
@canxin121 canxin121 force-pushed the fix-clear-residual-listeners-#8916 branch from e422726 to ff5c8de Compare February 24, 2024 07:37
@lucasfernog
Copy link
Member

I think there's a better way to do this. Handling this via a command called by the frontend might lead to race conditions.
What we could do instead is modify this closure and clear the listeners when the page starts to load.

@canxin121 canxin121 closed this Feb 27, 2024
@canxin121 canxin121 force-pushed the fix-clear-residual-listeners-#8916 branch from e1109ec to 3fb414b Compare February 27, 2024 09:56
…ners

merge `Test on page load clear js listeners` into default
@canxin121 canxin121 reopened this Feb 27, 2024
@canxin121
Copy link
Contributor Author

canxin121 commented Feb 27, 2024

I think there's a better way to do this. Handling this via a command called by the frontend might lead to race conditions. What we could do instead is modify this closure and clear the listeners when the page starts to load.

Very good suggestion, I hadn't thought of fixing this bug here before, and now that I've rewritten a new version which doesn't involve any front-end code and no more unlisten_all_js for tauri::command, it should work as expected.

demo.mp4

@vonPB
Copy link
Contributor

vonPB commented Feb 27, 2024

Been using this for today's development and had no issues at all :)

core/tauri/src/event/listener.rs Outdated Show resolved Hide resolved
core/tauri/src/event/mod.rs Outdated Show resolved Hide resolved
core/tauri/src/webview/mod.rs Outdated Show resolved Hide resolved
core/tauri/src/webview/mod.rs Outdated Show resolved Hide resolved
amrbashir
amrbashir previously approved these changes Feb 28, 2024
@amrbashir amrbashir merged commit e4463f0 into tauri-apps:dev Feb 28, 2024
19 checks passed
@amrbashir
Copy link
Member

Thank you

@samkearney
Copy link
Contributor

samkearney commented Mar 8, 2024

I'm not sure if this is expected, but this change seems to be causing some kind of race condition on initial load of a React page. I am calling listen() from the componentDidMount() function of a React component. From my debugging it seems like the following order of events is happening:

  1. Page begins to load, componentDidMount() is called, my code calls listen() and registers a listener
  2. Tauri considers the page to be "loaded". The on_page_load_handler is called, and hits a breakpoint inside the function that was added in this PR, unlisten_all_js(). This clears the listener my page had registered. (I verified this by inspecting the members in the debugger)
  3. Later, my backend code emits some events, but there is nothing to listen to them.

I am new to Tauri, so I may be doing something wrong, but is this expected?

@FabianLars
Copy link
Member

Hmm, i highly doubt that componentDidMount triggers before DOMContentLoaded. Not 100% sure but still feels wrong to me.

Can you share the code where you listen to and emit the event?

@samkearney
Copy link
Contributor

It's closed-source, but let me try to reduce it to something minimal. While I work on that, could you point me to any advice or best practices for places in the lifecycle of a React page where it makes the most sense to set up event listening?

@FabianLars
Copy link
Member

componentDidMount or useEffect with [] is indeed correct. You just have to unlisten to the event on unmount too. An example for that with useEffect (because i didn't use componentDidMount for years) looks like this

useEffect(() => {
  const unlisten = listen("someevent", console.log);
  
  return () => {
    unlisten.then( f => f() );
  }
}, []);

@samkearney
Copy link
Contributor

In making my minimal reproducer I realized I had several incorrect assumptions about my code. I think things are working now. Apologies for the spam.

@gulbanana
Copy link

Here's a repro demonstrating a bug where the listeners are cleaned up too late, destroying new callbacks that were just installed. For me, this works on Windows and on MacOS dev builds, but fails on MacOS production builds.
https://github.com/gulbanana/repro-tauri-macos-events

amrbashir added a commit that referenced this pull request Mar 11, 2024
…ered ids to be triggerd

fix regression introduced in #8930 , and reported in #8930 (comment)
lucasfernog added a commit that referenced this pull request Mar 12, 2024
…filtered ids to be triggered (#9151)

* fix(core/event): filter js listeners on rust side only then emit filtered ids to be triggerd

fix regression introduced in #8930 , and reported in #8930 (comment)

* Update .changes/core-js-unlisten-all-regression.md

Co-authored-by: Lucas Fernandes Nogueira <[email protected]>

* Discard changes to .changes/core-js-unlisten-all-regression.md

* object.defineproperty

* add change file [skip ci]

---------

Co-authored-by: Lucas Fernandes Nogueira <[email protected]>
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 this pull request may close these issues.

[bug] tauri v2.0.x 'unlisten' work strangely
7 participants