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.

Sign up for GitHub

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

Jump to bottom

Debounce page refreshes triggered via Turbo streams #1099

Merged
merged 1 commit into from
Jan 7, 2024
Merged

Debounce page refreshes triggered via Turbo streams #1099

merged 1 commit into from
Jan 7, 2024

Conversation

brunoprietog
Copy link
Collaborator

Fixes #1093

@brunoprietog brunoprietog mentioned this pull request Dec 4, 2023
Closed
edwinv
edwinv suggested changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@edwinv edwinv left a comment

Choose a reason for hiding this comment

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

Great work! A already started adding debounce as well, but this PR makes my work
superfluous. Hopefully my feedback helps to craft the best solution.

src/core/session.js Outdated Show resolved Hide resolved
src/tests/functional/page_refresh_stream_action_tests.js Outdated Show resolved Hide resolved
src/tests/helpers/page.js Outdated Show resolved Hide resolved
@brunoprietog brunoprietog force-pushed the debounce-stream-refreshes branch 2 times, most recently from e237023 to 624b9c6 Compare December 4, 2023 12:03
edwinv
edwinv approved these changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@edwinv edwinv left a comment

Choose a reason for hiding this comment

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

Much more readable now! 🙏 Other than a possible extra test case, I don't have feedback anymore.

src/tests/functional/page_refresh_stream_action_tests.js Show resolved Hide resolved
@brunoprietog brunoprietog force-pushed the debounce-stream-refreshes branch 2 times, most recently from 0a40b41 to 8a4bd7f Compare December 9, 2023 04:12
@brunoprietog brunoprietog mentioned this pull request Dec 15, 2023
Closed
ArturT
ArturT reviewed Dec 15, 2023
View reviewed changes
src/core/session.js Outdated Show resolved Hide resolved
@dhh dhh requested a review from jorgemanrubia December 28, 2023 16:16
jorgemanrubia
jorgemanrubia reviewed Dec 29, 2023
View reviewed changes
Copy link
Member

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

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

@brunoprietog Thanks for working on this. I added some minor comments.

src/core/session.js Outdated Show resolved Hide resolved
src/tests/functional/page_refresh_stream_action_tests.js Outdated Show resolved Hide resolved
@brunoprietog brunoprietog force-pushed the debounce-stream-refreshes branch from 8a4bd7f to aecd2ac Compare December 29, 2023 19:50
@brunoprietog
Copy link
Collaborator Author

@jorgemanrubia I've added the Turbo.session.pageRefreshDebouncePeriod setting but I had to change the implementation slightly, so that changing the value changes the debounce delay again. Can you think of a more elegant way to do this or is it ok?

jorgemanrubia
jorgemanrubia reviewed Jan 7, 2024
View reviewed changes
Copy link
Member

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @brunoprietog. I added a few more comments, but good call noticing we need to account for the change of the property when scheduling the debounce logic.

src/tests/unit/stream_element_tests.js
@@ -177,7 +177,7 @@ test("test action=refresh", async () => {
const element = createStreamElement("refresh")
subject.append(element)

await nextBeat()
await sleep(250)
Copy link
Member

Choose a reason for hiding this comment

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

Could we use nextPageRefresh instead of the remaining sleep(250)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That helper uses Playwright and we don't have access to it here, that's why I haven't changed it. But you're right that it doesn't read well.

src/core/session.js Outdated Show resolved Hide resolved
src/core/session.js Show resolved Hide resolved
@brunoprietog brunoprietog force-pushed the debounce-stream-refreshes branch from aecd2ac to ac5f854 Compare January 7, 2024 11:49
@brunoprietog
Debounce page refreshes triggered via Turbo streams
2147a14 
Fix Turbo 8 refresh debounce in frontend hotwired#1093
@brunoprietog brunoprietog force-pushed the debounce-stream-refreshes branch from ac5f854 to 2147a14 Compare January 7, 2024 11:58
jorgemanrubia
jorgemanrubia approved these changes Jan 7, 2024
View reviewed changes
Copy link
Member

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

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

Thanks @brunoprietog

@jorgemanrubia jorgemanrubia merged commit 6fc2f38 into hotwired:main Jan 7, 2024
1 check passed
@brunoprietog brunoprietog deleted the debounce-stream-refreshes branch January 7, 2024 12:11
@YoutacRandS-VA YoutacRandS-VA mentioned this pull request Jun 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArturT ArturT ArturT left review comments

@edwinv edwinv edwinv approved these changes

@jorgemanrubia jorgemanrubia jorgemanrubia approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Turbo 8 refresh debounce in frontend
4 participants
@brunoprietog @jorgemanrubia @edwinv @ArturT