Add API for custom time functions implementation

[movpushmov]

Proposal

Now patronum delay, interval, debounce, time works with global time functions (window.setTimeout, window.setInterval, window.Date.now) what make testing of some features impossible. The solution is create API where developer can manually set implementations of setTimeout/setInterval

Someting like this:

const $timers = createStore({ setTimeout: (cb) => cb() });

const startDelay = createEvent();
const triggered = delay({ source: startDelay, timeout: 200,  timers: $timers });

Motivation

1. Developer can set their own implementations of time functions (this make testing very easy)
2. This solution much better than jest fake timers, 'cause it can't create race condition even in theory

Related links:

1. Jest mock race condition
2. Jest async race condition

[AlexandrHoroshih]

Hello @movpushmov !

Thank you for the RFC! I would like to discuss the API Design of it 🤔

I see the problem with the custom timers API, that it is kinda replicates that of Jest/Vitest - which is probably not desirable, as those are already built-in into those frameworks, and are much more complicated and feature-rich. We don't really want to compete with Jest or Vitest there 😅

I would suggest something simpler, like just to expose internal effects of timer-based operators, like this:

const scope = fork({
 handlers: [
   [debounce.timerFx, myMock],
   [throttle.timerFx, myMock],
   [delay.timerFx, ...],
 ]
})

☝️ So this work much like fetchFx of Farfetched - a special escape-hatch for edge-cases like "buggy test framework" or "weird runtime"

I like this approach more, since it is simple, straightforward and much easier for mainteiners of patronum to handle

What do you think?

[movpushmov]

Hello @movpushmov !

Thank you for the RFC! I would like to discuss the API Design of it 🤔

I see the problem with the custom timers API, that it is kinda replicates that of Jest/Vitest - which is probably not desirable, as those are already built-in into those frameworks, and are much more complicated and feature-rich. We don't really want to compete with Jest or Vitest there 😅

I would suggest something simpler, like just to expose internal effects of timer-based operators, like this:

const scope = fork({
 handlers: [
   [debounce.timerFx, myMock],
   [throttle.timerFx, myMock],
   [delay.timerFx, ...],
 ]
})

☝️ So this work much like fetchFx of Farfetched - a special escape-hatch for edge-cases like "buggy test framework" or "weird runtime"

I like this approach more, since it is simple, straightforward and much easier for mainteiners of patronum to handle

What do you think?

sounds very good)

[sergeysova]

I would suggest something simpler, like just to expose internal effects of timer-based operators, like this:

const scope = fork({
 handlers: [
   [debounce.timerFx, myMock],
   [throttle.timerFx, myMock],
   [delay.timerFx, ...],
 ]
})

Looks good to me!

[igorkamyshev]

Hey!

Could you elaborate on motivation a bit? I do not see any reason for adding a new API based on provided motivation.

Developer can set their own implementations of time functions (this make testing very easy)

Any modern test-runner allows users to mock timers by something like vi.useFakeTimers(). What is wrong with this API? Why does it not solve your problem? What is this problem?

This solution much better than jest fake timers, 'cause it can't create race condition even in theory

If we are going to solve a particular bug/race in Jest (single test runner), shall we address this RFC to the Jest repository? It seems quite overwhelming to introduce a new API to a library to fix a bug in some other tool.

So, I would like to hear more motivation for adding a new API because any new API increases library complexity for users (they need to learn more APIs — vi.useFakeTimers() AND our custom API for timers mocking) and maintainers (any new feature should be aligned with this API).

Furthermore, I read documentation provided in #327 and I cannot catch it. Can I completely replace vi.useFakeTimers() with the new API? What should I do? What is the replacement for shouldAdvanceTime: true? Should I write it myself? How? Why? Sorry, but there are too many questions without answers.

Before this motivation clarification and API design description, I suggest holding PR https://github.com/effector/patronum/pull/327 🙏

[dima117]

Hello, @igorkamyshev! If you don't mind, I'll help @movpushmov answer the questions.

Any modern test-runner allows users to mock timers by something like vi.useFakeTimers(). What is wrong with this API? Why does it not solve your problem? What is this problem?

useFakeTimers solves the problem of isolated code execution that depends on global variables (browser API). Code using global variables can only be run in real isolation in a separate process, which is unacceptable. useFakeTimers simulates isolated code execution within a single process based on assumptions:

• the tests are run sequentially and do not overlap
• the tested code does not do side effects before and after the test

Both of these assumptions can be violated and debugging such tests is time-consuming and painful.

For example, imagine that the code under test remembers the current time or the now function in a global variable at the top level of the module. This means that the global variable will be remembered when importing the module, before calling useFakeTimers. This means that after calling useFakeTimers, part of the code under test will use a fake implementation, and the other part will use the real implementation stored in a global variable inside the module.

If we are going to solve a particular bug/race in Jest (single test runner), shall we address this RFC to the Jest repository?

It seems that we should not solve the problem in useFakeTimers, but instead eliminate the reason why we had to use it. To do this, you need to rewrite the code so that it does not use global variables. For example, you can move the global API call inside the effect. But, unfortunately, if the patronum library is used, then this cannot be done, because it depends internally on global variables and does not allow them to be redefined.

any new API increases library complexity for users and maintainers (any new feature should be aligned with this API).

The essence of the proposal is to support the patronum abstraction layer over the browser API. To do this, you will need to explicitly describe it. If there are questions about specific parts of the API, this can be discussed. It seems to describe the interfaces and add options to override them is a small change that solves the problem cheaply. Also note that adding an abstraction layer does not impose additional responsibility on the library authors, since the code already depends on this API. It's just not explicitly described.

[dima117]

Hello! @igorkamyshev @sergeysova @AlexandrHoroshih Do you have any more questions? What are the next steps?