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

Implement requestIdleCallback #218

Closed
benjamingr opened this issue Oct 17, 2018 · 13 comments
Closed

Implement requestIdleCallback #218

benjamingr opened this issue Oct 17, 2018 · 13 comments

Comments

@benjamingr
Copy link
Member

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
Copy link
Contributor

shdq commented Oct 17, 2018

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

@benjamingr
Copy link
Member Author

benjamingr commented Oct 17, 2018

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

  • The first step would be to write code that uses requestIdleCallback in the browser and see how it works. Get a feel for what it does etc.
  • The second step would be to think about what behaviour people expect when requestIdleCallback is called without a timer and what lolex should do in that case (call it never? call it but only when timers are run "to the end" and call it last? call it before timers but after microtasks?)
  • The third step would be to write tests for it (in the file test/lolex-test.js) for said behaviour and open a WIP PR with that
  • The fourth step would be to go to src/lolex-src.js and add a test for it (see how for example nextTick or performance presence is tested)
  • The fifth step would be to - if it is present add the code - see how runJobs works for instance. Add requestIdleCallback to the clock if it is present and then add code to treat it according to the desired behaviour. For example if it has a timeout we probably want to run it the same way we run timers (as a timer).
  • Then we can do back-and-forth on the PR.

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
Copy link
Member Author

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
Copy link
Contributor

shdq commented Oct 17, 2018

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
Copy link
Member Author

@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
Copy link
Contributor

shdq commented Oct 18, 2018

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
Copy link
Member Author

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
Copy link
Member

SimenB commented Oct 18, 2018

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
Copy link
Member Author

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
Copy link
Contributor

shdq commented Oct 18, 2018

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
Copy link
Contributor

shdq commented Oct 19, 2018

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
Copy link
Member Author

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
Copy link

stale bot commented Dec 18, 2018

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.

@stale stale bot added the stale label Dec 18, 2018
@stale stale bot closed this as completed Dec 26, 2018
benjamingr added a commit that referenced this issue Jan 20, 2019
[WIP] Issue #218 – `requestIdleCallback` implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants