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

Add debounce on reload-listener.js #222

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

Mte90
Copy link

@Mte90 Mte90 commented Nov 10, 2023

Ref: #22

I am not sure about the ms for the debounce.

Copy link
Owner

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test this? It looks like it won’t work as the debounce calls won’t share the timer. Also testing would help find an appropriate timeout.

@adamchainz
Copy link
Owner

Please also add a changelog note.

@Mte90
Copy link
Author

Mte90 commented Nov 14, 2023

So the debounce function is executed but not the timeout, my guess is that is something that in a worker act differently...

@Mte90
Copy link
Author

Mte90 commented Nov 21, 2023

So for my investigation the debounce doesn't work as it is inside a sharedworker, so the timer is resetted everytime and not kept in memory.
I don't have experience with workers and I am not able to investigate further.

@Mte90
Copy link
Author

Mte90 commented Nov 24, 2023

Other thread that confirms that the shared worker doesn't remember the previous timeout so the debounce doesn't work:

@Mte90 Mte90 closed this Jul 4, 2024
@adamchainz
Copy link
Owner

adamchainz commented Jul 4, 2024

I’m pretty sure the client-side debounce will work, because the worker process is long-lived. This PR currently edits reload-listener.js, which runs in the browser tab. It should edit reload-worker.js, the worker script.

Keeping open so you or I or someone can pick it up.

@adamchainz adamchainz reopened this Jul 4, 2024
@Mte90
Copy link
Author

Mte90 commented Jul 4, 2024

I don't have time to work on that so this is why I closed it as there is already a ticket about it.

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.

2 participants