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

Expose way to run just microtasks #147

Closed
benjamingr opened this issue Dec 24, 2017 · 18 comments
Closed

Expose way to run just microtasks #147

benjamingr opened this issue Dec 24, 2017 · 18 comments

Comments

@benjamingr
Copy link
Member

For jest integration - we should consider exposing a way to run only microtasks (nextTick) - this effectively means exporting runJobs.

There are no other things currently shimmed in lolex with microtask semantics (promises - no because of async functions, MutationObservers no). Since everything defers based on nextTick semantics that should be fine for jest inside jsdom.

CC @SimenB

@SimenB
Copy link
Member

SimenB commented Dec 24, 2017

We also have the inverse in Jest - just running macrotasks.

Another diff in this regard is that our tick (called advanceTimersByTime) just triggers timers, it doesn't touch microtasks. Again, I'm not sure in what instances the distinction is useful, but I suppose it exists for a reason 🙂

See Jests docs (from the linked paragraph and down) for a rundown of what we do today. http://facebook.github.io/jest/docs/en/jest-object.html#jestrunallticks
Any obvious issues popping up?

@benjamingr
Copy link
Member Author

I don't understand why running just macroticks and not microticks makes sense. (Since it would never be regular execution flow). Any motivation on why this is required?

@SimenB
Copy link
Member

SimenB commented Dec 24, 2017

@cpojer (who's on vacation, so might be a bit before we get an answer) any particular use cases for this?

@cpojer
Copy link

cpojer commented Dec 24, 2017

I think running both in one call works fine with me, but I cannot confirm without testing this on the FB codebase. I can check usage of advanceTimersByTime at FB in early January to confirm.

@benjamingr
Copy link
Member Author

benjamingr commented Feb 15, 2018

Taking a look

Update: PR

@stale
Copy link

stale bot commented Apr 16, 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 Apr 16, 2018
@benjamingr
Copy link
Member Author

Not really stale, waiting for review on PR still

@stale stale bot removed the stale label Apr 16, 2018
@fatso83
Copy link
Contributor

fatso83 commented Apr 16, 2018

@benjamingr I haven't noticed the PR at all: do you have all required privileges to merge now, btw?

@fatso83
Copy link
Contributor

fatso83 commented Apr 16, 2018

@benjamingr You had been added to the lolex-core team ages ago, but I had forgotten to give lolex-core rights to do anything on the lolex repo. You should be good to write and approve stuff now! Sorry for delaying this, but a new family situation 🍼 arose around the time of this PR.

Don't be a stranger to pestering me on email, btw, if I should be blocking something.

@benjamingr
Copy link
Member Author

@fatso83 congratulations! Really no rush here or sense of urgency - family first and thanks :)

@SimenB
Copy link
Member

SimenB commented May 8, 2018

@benjamingr I think I've found an issue with the implementation - it doesn't guard against recursion in the same way the timers do. Jest has the following test, which never completes when using this new function:

it('throws before allowing infinite recursion', () => {
  const global = {
    Date,
    clearTimeout,
    process: {
      nextTick: () => {},
    },
    setTimeout,
  };

  const timers = new FakeTimers({
    global,
    maxLoops: 100,
    moduleMocker,
    timerConfig,
  });

  timers.useFakeTimers();

  global.process.nextTick(function infinitelyRecursingCallback() {
    global.process.nextTick(infinitelyRecursingCallback);
  });

  expect(() => {
    timers.runAllTicks();
  }).toThrow(
    new Error(
      "Ran 100 ticks, and there are still more! Assuming we've hit an " +
        'infinite recursion and bailing out...',
    ),
  );
})

I think runMicrotasks should also respect the loopLimit argument.

@benjamingr
Copy link
Member Author

process.nextTick doesn't itself guard against this though - even without anything fake:

process.nextTick(function foo() { process.nextTick(foo); }); // freezes Node

Since process.nextTick is just a synchronous call after the current context rather than actually crossing a "boundary" between C++/JavaScript. Non-fake timers on the other hand would never "freeze" .

Note that I'm not disagreeing with the idea of adding this - We can add bailout logic if you think it's important (or use loopLimit). I was just explaining why it wasn't the case for #156 - runJobs could certainly handle this case.

@benjamingr benjamingr reopened this May 8, 2018
@benjamingr
Copy link
Member Author

Going to reopen since it's being discussed

@SimenB
Copy link
Member

SimenB commented May 8, 2018

Interesting, didn't know that! A bail seems more useful than a freeze, especially in a testing situation (as it prevents tests from timing out), but matching real behavior (beyond manipulating time) is really compelling as well.

@cpojer thoughts on this?

@cpojer
Copy link

cpojer commented May 8, 2018

Regardless of what's better, we cannot change behavior for existing Jest tests at this time. If we want to use lolex for Jest, the initial version must 100% match what Jest has right now. We can make incremental breaking changes later.

@benjamingr
Copy link
Member Author

I'll make a PR @SimenB @cpojer

@benjamingr
Copy link
Member Author

@SimenB let me know if the fix in #172 works for jest

@benjamingr
Copy link
Member Author

This is fixed in #172

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

4 participants