sinonjs / fake-timers

Fake setTimeout and friends (collectively known as "timers"). Useful in your JavaScript tests. Extracted from Sinon.JS
BSD 3-Clause "New" or "Revised" License
806 stars 107 forks source link

Implement `requestIdleCallback` #218

Closed benjamingr closed 5 years ago

benjamingr commented 6 years ago

This is an issue for a new contributor to pick up unless someone has an urgent requirement lolex adds this.

requestIdleCallback isn't implemented in lolex. Since it's timing related - we should probably support it.

This ticket is to implement it in lolex (it will be guided if you're new and not sure you can do it!)

**What did you expect to happen?

var called;
requestIdleCallback(function () { called = true; }, 100);
clock.tick(101);
assert(called); // passes

What actually happens

lolex does not override requestIdleCallback

shdq commented 6 years ago

Hello, I'm interested in solving this issue. Can you guide me, please?

benjamingr commented 6 years ago

Hey sure, @shdq - thank you for offering to help!

I estimate this will be about 10 hours of work. Is this something you feel comfortable committing to? (If so, do you feel comfortable committing to in a 2 week timeframe?)

benjamingr commented 6 years ago

Also, if any step is unclear (if you want to commit to this which by any means you're not obligated to since it's volunteering your time to open source) - by all means ask about it :)

shdq commented 6 years ago

It's ok, I have enough time to work on it. I think it will be a lot of questions from me as a newcomer.

benjamingr commented 6 years ago

@shdq thanks! I'm removing the "help wanted" label and I welcome the lots of questions you may have 😄

Questions are welcome + it's an opportunity for us to clarify things.

shdq commented 6 years ago

I did some research about requestidlecallback and tried it. According to wc3 spec idle callbacks algorithm default behaviour of requestIdleCallback without timeout option idle callbacks have "no guarantee" to execute in busy app because user agent may find higher priority work to do and delay them. That's why it has to be used only for non-essential tasks.

About timeout option, seems like you should use it only when you really need to, because it can lead to lags and bad user experience if idle task will be executed when where isn't have enough time for it and can affect performance.

To find idle period, algorithm has several steps and one of them is to:

Spin the event loop until all the task queues and the microtask queue associated with event_loop are empty.

So, i think in lolex case requestIdleCallback (without timeout option) should be executed last after all tasks (we consider timers as tasks) and microtasks.

What do you think, @benjamingr?

benjamingr commented 6 years ago

So, i think in lolex case requestIdleCallback (without timeout option) should be executed last after all tasks (we consider timers as tasks) and microtasks.

That sounds good to me 👍 To be clear that implies that tick(n) shouldn't execute it but runAll should (and those two should be test cases).

If the timeout option exists on the other hand it should run according to that as a timer right?

SimenB commented 6 years ago

Seems odd that if we tick say 1000ms between 2 timers that it's not considered "idle" in between.

setTimeout(() => {
  console.log('timeout');
}, 500);

requestIdleCallback(() => {
  console.log('idle');
});

idle will print pretty much immediately in real code. I don't have a good heuristic of the top of my head, but what about e.g. "next timer/task is more than 100ms away, fire an idle callback"?

benjamingr commented 6 years ago

idle will print pretty much immediately in real code. I don't have a good heuristic of the top of my head, but what about e.g. "next timer/task is more than 100ms away, fire an idle callback"?

That also works - to be fair I'm more concerned about the case where the timeout is passed in which case we have a real guarantee.

shdq commented 6 years ago

If the timeout option exists on the other hand it should run according to that as a timer right?

In native implementation the idle and timeout callbacks are raced and cancel each other – e.g. if the idle callback is scheduled first, then it cancels the timeout callback, and vice versa.

what about e.g. "next timer/task is more than 100ms away, fire an idle callback"?

Good point, timeRemaining() for the idle period is 50ms max, so it can be even two idle periods in a row if other important work won't occur.

shdq commented 6 years ago

Hey, @benjamingr I have a questions about git workflow. I suppose to do this:

  1. Fork lolex repo;
  2. Create new feature branch;
  3. Make a commit with tests in test/lolex-test.js;
  4. Create a pull request to upstream with [WIP] in title; What next?

Would it be possible to keep committing in this PR? Can I add at least one test and make a WIP PR and keep working in it with your help?

About tests. I'm not very familiar with it, in test/lolex-test.js as I understand there are mocha tests. For requestIdleCallback there are some tests similar to requestAnimationFrame, like "no arguments", "return numeric id", "unique id", etc. Am I right? Can I add them in WIP PR for a start?

What kind of tests in fourth step? Any help, references?

And we still not decided with the behavior of requestIdleCallback in lolex. And I think we should also add cancelIdleCallback, yeah?

Thank you.

benjamingr commented 6 years ago

Would it be possible to keep committing in this PR? Can I add at least one test and make a WIP PR and keep working in it with your help?

Yes, you can do that by committing to that branch on your repository.

About tests. I'm not very familiar with it, in test/lolex-test.js as I understand there are mocha tests. For requestIdleCallback there are some tests similar to requestAnimationFrame, like "no arguments", "return numeric id", "unique id", etc. Am I right? Can I add them in WIP PR for a start?

Sure.

What kind of tests in fourth step? Any help, references?

A test there means "test for existence" like an requestIdleCallbackPresent variable that checks for the existence of requestIdleCallback.

And we still not decided with the behavior of requestIdleCallback in lolex. And I think we should also add cancelIdleCallback, yeah?

Sure :)

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.