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

fetch support delay #2602

Closed
wants to merge 3 commits into from
Closed

fetch support delay #2602

wants to merge 3 commits into from

Conversation

mikicho
Copy link
Contributor

@mikicho mikicho commented Mar 8, 2024

  • I had to patch the AbortSignal.timeout to get the delay for throwing the 'timeout' error without actually waiting for it.
  • I'm wondering if it's worth it... signal is very general, and my current solution is for a very specific use case (AbortSignal.timeout function only), I'm not certain if it belongs in Nock.
  • Currently, I reject the promise on the timeout event, which means the signal isn't really aborted. Is there an easy way to update the kAborted to true and kReason? We may override the signal with an aborted signal, but I'm afraid it would be fragile:
clientRequest.on('timeout', () => {
  init.signal = AbortSignal.abort(new DOMException('The operation was aborted due to timeout', 'TimeoutError'))
  reject(init.signal.reason)
})

@gr2m @Uzlopak WDYT?

@gr2m
Copy link
Member

gr2m commented Mar 11, 2024

I tried to wrap my head around it but I don't have the mental capacity to think this through right now 🤕 I'd be okay with removing functionality from nock if that removes a blocker to ship beta, and then open a discussion on how to bring it back. The folks depending on it will then show up and share their use cases, and might even collaborate on a solution.

@Uzlopak
Copy link
Member

Uzlopak commented Mar 12, 2024

I dont understand why it needs Abortcontroller? Does the normal delay also use a Abortcontroller? I would have expected a await new Promise(resolve => setTimeout(resolve, delay))

@mikicho
Copy link
Contributor Author

mikicho commented Apr 8, 2024

After some thought, I think the delay connection doesn't make much sense with Fetch API, which doesn't provide this level of granularity.

Users should use time fakers instead if they don't want to wait.

Another option we may consider... Nahh probably a bad idea Another option we may consider is to expose a new global property that enables users to define their time faker, which later we can use in the interception logic:
// setup
nock.setFakeTimer(sinon.useFakeTimers())

it(..., () => {
   nock(..).get(...).delay(2000).reply()
})

In Nock's interception logic, we can do the following:

nock.getFakeTimer().tick(interceptor.delay + 100);

WDYT?

@gr2m
Copy link
Member

gr2m commented Apr 9, 2024

I would prefer not to make fake timers an API we support with nock. They are a recipe for headaches. I would rather remove the nock API for delays altogether.

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

Successfully merging this pull request may close these issues.

None yet

3 participants