Skip to content

Conversation

@stephanmoneybird
Copy link

@stephanmoneybird stephanmoneybird commented Jul 2, 2025

Ad support for tracking asynchronous work scheduled via setTimeout. Any setTimeout call with a delay less than 5000ms is wrapped so that capybara-lockstep knows to wait for that work to complete before proceeding.

Wait for asynchronous work scheduled through `setTimeout` with delays under 5000ms. Wrapped `window.setTimeout` and`window.clearTimeout` to track and signal ongoing work.
@triskweline
Copy link
Member

triskweline commented Jul 3, 2025

Thanks for looking into the synchronization of timeouts @stephanmoneybird .

I'm afraid merging it like this would freeze synchronization in many apps I have seen, which use a pattern like this:

async function startPinging() {
  await fetch('/ping') // or do any other async work
  setTimeout(startPinging, 1000)
}

startPinging()

This is the correct implementation of polling, as setInterval() wouldn't take into account the time spent waiting for the network. However, since there's practically always a setTimeout() pending, synchronization would run for a long time. Also we don't want to synchronize that use of a timeout.

What we could do is to lower the threshold so something like 10ms. This would synchronize all the cases where people use setTimeout() to push a function into the next task (e.g. to wait for queued promise callbacks to run). I think it's unlikely to see code that have a 10ms timeout going at all times.

It would probably be nice to have a configuration for the threshold, maybe like this:

Capybara::Lockstep.wait_timeout_max_delay = 10   # default
Capybara::Lockstep.wait_timeout_max_delay = 2000 # custom setting
Capybara::Lockstep.wait_timeout_max_delay = -1   # disable setTimeout() sync

We already have precedent for a config setting in Capybara::Lockstep.wait_tasks, and there's already code in place to move the Ruby-side config into the JavaScript helper.

I only have two comments about the diff itself:

  • I didn't understand why we generate a random waitId, and maintain the waitId => timeoutId mapping. Can't we track timeouts using a Set of timeoutID? They are already unique.
  • The startWork() and stopWork() arguments are the actual strings printed to the console, and we should log something more descriptive here. We can use startWork('setTimeout()') and stopWork('setTimeout()') here. The job names don't need to be unique.

Thanks for writing a test!

@stephanmoneybird
Copy link
Author

I've processed your feedback, let me know what you think!

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