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

fix(wait-for): Don't queue microtasks after condition is met #1073

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 79 additions & 2 deletions src/__tests__/wait-for.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,17 @@ function deferred() {
return {promise, resolve, reject}
}

let originalConfig
beforeEach(() => {
originalConfig = getConfig()
})

afterEach(() => {
configure(originalConfig)
// restore timers
jest.useRealTimers()
})

test('waits callback to not throw an error', async () => {
const spy = jest.fn()
// we are using random timeout here to simulate a real-time example
Expand Down Expand Up @@ -138,7 +149,6 @@ Ignored nodes: comments, <script />, <style />
})

test('should delegate to config.getElementError', async () => {
const originalConfig = getConfig()
const elementError = new Error('Custom element error')
const getElementError = jest.fn().mockImplementation(() => elementError)
configure({getElementError})
Expand All @@ -153,7 +163,6 @@ test('should delegate to config.getElementError', async () => {

expect(getElementError).toBeCalledTimes(1)
expect(error.message).toMatchInlineSnapshot(`Custom element error`)
configure(originalConfig)
})

test('when a promise is returned, it does not call the callback again until that promise rejects', async () => {
Expand Down Expand Up @@ -255,3 +264,71 @@ test('the real timers => fake timers error shows the original stack trace when c

expect((await waitForError).stack).not.toMatch(__dirname)
})

test('does not work after it resolves', async () => {
jest.useFakeTimers('modern')
let context = 'initial'
configure({
// @testing-library/react usage to ensure `IS_REACT_ACT_ENVIRONMENT` is set when acting.
unstable_advanceTimersWrapper: callback => {
const originalContext = context
context = 'act'
try {
const result = callback()
// eslint-disable-next-line jest/no-if
if (typeof result?.then === 'function') {
const thenable = result
return {
then: (resolve, reject) => {
thenable.then(
returnValue => {
context = originalContext
resolve(returnValue)
},
error => {
context = originalContext
reject(error)
},
)
},
}
} else {
context = originalContext
return undefined
}
} catch {
context = originalContext
return undefined
}
},
asyncWrapper: async callback => {
const originalContext = context
context = 'no-act'
try {
await callback()
} finally {
context = originalContext
}
},
})

let data = null
setTimeout(() => {
data = 'resolved'
}, 100)

await waitFor(
() => {
if (data === null) {
throw new Error('not found')
}
},
{interval: 50},
)

expect(context).toEqual('initial')

await Promise.resolve()

expect(context).toEqual('initial')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was no-act without the change. We had the following schedule-resolution order:

Without this change:

  1. start asyncWrapper
  2. start unstable_advanceTimersWrapper
  3. leave asyncWrapper
  4. leave unstable_advanceTimersWrapper

With this change:

  1. start asyncWrapper
  2. start unstable_advanceTimersWrapper
  3. leave unstable_advanceTimersWrapper
  4. leave asyncWrapper

In other words, before asyncWrapper and unstable_advanceTimersWrapper were interleaved instead of nested.

})
4 changes: 4 additions & 0 deletions src/wait-for.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ function waitFor(
// an entire day banging my head against a wall on this.
checkCallback()

if (finished) {
break
}

// In this rare case, we *need* to wait for in-flight promises
// to resolve before continuing. We don't need to take advantage
// of parallelization so we're fine.
Expand Down