medikoo / memoizee

Complete memoize/cache solution for JavaScript
ISC License
1.73k stars 61 forks source link

Check for undefined before accessing timeout #106

Open supersonicclay opened 4 years ago

medikoo commented 4 years ago

@supersonicclay great thanks for that PR. in which engine did you observe that setTimeout() returninng undefined ? By spec it is guranteed to return some value -> https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/setTimeout

supersonicclay commented 4 years ago

It is super weird. I was getting it when running some unit tests on my project. They're jest tests running in the browser. When running in node.js the same tests work. But putting in that null check fixed it.

medikoo commented 4 years ago

@supersonicclay it might be then case of Jest exposing not standard setTimeout, that generally calls for issues. Maybe it's worthwhile to open an issue there?

supersonicclay commented 4 years ago

I can definitely dive down that rabbit hole, but it might be a deep one trying to zero in on a small reproducible example. But since this change is small and benign, I thought it may be the path of least resistance.

medikoo commented 4 years ago

I thought it may be the path of least resistance.

I believe it's way better to patch bugs directly at its source.

I wouldn't be surprised if it's not only this library which crashes dirty in Jest environment. Fixing it at Jest will make it fixed for all eventual cases, and all eventual code that assumes that setTimeout actually returns a value

david-taggun commented 3 years ago

For anyone else facing this problem.. perhaps this should be raised as an issue and closed so that people can look it up.

@medikoo is right in that the Jest environment does modify the behaviour of setTimeout which causes this issue to occur. The solution is to use Jest fakeTimers in your tests and then force them to run after each evoked memoizee method, this will ensure that the setTimeouts return a value that should avoid this error.

describe('ClassThatsMemoized.validate', () => {
  jest.useFakeTimers();
  const c = new ClassThatsMemoized();

  test('dont throw', async () => {
   /**
    * memoize(this.validate, {
    *    promise: true,
    *    length: 2,
    *    maxAge: 3600000,
    * });
    */
    const result = await c.validate('', '');
    jest.runAllTimers();
    expect(result).toBeTruthy();
    expect(result).toMatchSnapshot();
  });
});

This issue is discussed in greater detail here: https://github.com/facebook/jest/issues/3211