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

Check for undefined before accessing timeout #106

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

supersonicclay
Copy link

No description provided.

@medikoo
Copy link
Owner

medikoo commented Sep 4, 2019

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

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

medikoo commented Sep 4, 2019

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

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

medikoo commented Sep 4, 2019

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

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: jestjs/jest#3211

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants