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.

Sign up for GitHub

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

Add Suspense and ErrorBoundary to better support suspending from hooks #35

Merged
merged 1 commit into from
Apr 16, 2019
Merged

Add Suspense and ErrorBoundary to better support suspending from hooks #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
60 changes: 46 additions & 14 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,37 @@
import React from 'react'
import React, { Suspense } from 'react'
import { render, cleanup, act } from 'react-testing-library'

function TestHook({ callback, hookProps, children }) {
try {
Copy link
Contributor

@ntucker ntucker Apr 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

If you kept this block an just re-threw you could also grab any promises to store in a third result value of 'promise' or the like. While waitForNextUpdate() will work in most cases, inspecting the promise thrown would be useful in test cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

(If you rethrow the exact same error it will preserve the original stack trace.)

Copy link
Member Author

Choose a reason for hiding this comment

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

The intention of removing this and using a Suspense and ErrorBoundary wrapper was to protect this library from having to deal with the internals of how error handling and suspended rendering actually works. The fact that a thrown promise is what triggers react to suspend is an implementation for React, rather than something we should be relying on.

Copy link
Contributor

Choose a reason for hiding this comment

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

you're still setting error property though, right? Seems like you are somewhat depending on react in that manner. regardless - this is a react hooks testing library. very specific to how react works.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, error is being set, but it's coming from the exposed componentDidCatch API, so we don't care how React gets it to that point, just deal with it when it gets there. When we look at the Suspense equivalent, React does not expose this to us in any way, so if we want to handle it, we essentially need to recreate react's internal logic in order to treat it the same way. If React changes the specifics on how it deals with promises, we would also need to change.

Copy link
Member Author

@mpeyper mpeyper Apr 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

if waitForNextUpdate() works

It should. I'd rather fix holes where it doesn't than maintain custom suspense handling.

  • the promise to be caught so it doesn't throw above

This solution would not end up throwing out of the renderHook function, if that's what you mean?

  • a mechanism to see when it is called

Can you unpack this a bit more? Are you concerned about when an update was triggered by something other than the promise resolving, or something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

One path is that no promise is thrown from the hook - I want to test that it actually threw a promise, and THEN did something else

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I'm questioning the value of having that much pwoer to inspect the internals of what's happening in your hook (i.e. white box test) vs. just asserting the output from the given inputs (i.e. black box test).

In my experience, white box test are very fragile to changes in the system and need constant updating as the internals shift around (e.g. you might change the condition for when the promise is thrown, so now would need to move things around in your tests to match the change). The black box tests are more likely to continue to work so, long as the contract of the inputs and outputs hasn't changes (otherwise the test must be changed anyway in both testing styles).

In cases where you need more control over when and how the promise thrown, then you may be better off extracting it into some form of factory pattern and mocking that behaviour in the hook test, and testing those variations of the factory seperately to the hook itself.

As an escape hatch, if you really need access to the promise itself, I think you could just catch it in the hool callback itself

let promise
const { result } = renderHook(() => {
  promise = null // assuming you want to reset it between renders
  try {
    useSomething()
  } catch (e) {
    if (e.then) {
      promise = e
    }
  }
})

const promiseValue = await promise

expect(promiseValue).toBe('whatever')

I'm still unsure how that is more useful than

const { result, waitForUpdate } = renderHook(() => useSomething())

await waitForUpdate

expect(result.current).toBe('whatever you expect after the prmise resolved')

As in both cases

  • if the promise resolves, and the expectation is met, the the test passes
  • if the promise resolves, and the expectation is not met, the test fails
    • in the first example all you would know the promise value changed
    • in the second example you would only know something went wrong to affect the output
  • if the promise rejects, the test fails with the thrown error
  • if the promise is not thrown, the test will fail
    • in the first example it would probably be about trying to await null
    • in the second example the test would timeout waiting for an update that never comes

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm also leaning away from explicitly handling supsense as it would get very difficult to manage if you had a hook throw a promise, and then once resolved throw another promise, etc. Suspense would keep that component suspending untill it eventually rendered something, whereas keeping track of each one and what they were up to could get complicated.

Copy link
Contributor

@ntucker ntucker Apr 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I mean, I would argue that suspense is very much an explicit interface and not an implementation detail - as anyone using the hook that throws promises will need to define suspense boundaries themselves. It's not about how but if.

Yes it could throw many times - that should be up to the test writer to understand exactly what that behavior should be - or if it even cares at all. I agree in many cases you won't care, and you can simply waitForUpdate. But there are certainly cases where you will.

(I responded slowly this time due to company hackathon last week and then I was out traveling over weekend)

children(callback(hookProps))
} catch (e) {
children(undefined, e)
children(callback(hookProps))
return null
}

class ErrorBoundary extends React.Component {
Copy link
Member Author

Choose a reason for hiding this comment

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

I wish there was a non-class way to handle error boundaries

constructor(props) {
super(props)
this.state = { hasError: false }
}

static getDerivedStateFromError() {
return { hasError: true }
}

componentDidCatch(error) {
this.props.onError(error)
}

componentDidUpdate(prevProps) {
if (this.props != prevProps && this.state.hasError) {
this.setState({ hasError: false })
}
}

render() {
return !this.state.hasError && this.props.children
}
}

function Fallback() {
return null
}

Expand All @@ -27,27 +52,34 @@ function resultContainer() {
}
}

const updateResult = (val, err) => {
value = val
error = err
resolvers.splice(0, resolvers.length).forEach((resolve) => resolve())
}

return {
result,
addResolver: (resolver) => {
resolvers.push(resolver)
},
updateResult: (val, err) => {
value = val
error = err
resolvers.splice(0, resolvers.length).forEach((resolve) => resolve())
}
setValue: (val) => updateResult(val),
setError: (err) => updateResult(undefined, err)
}
}

function renderHook(callback, { initialProps, ...options } = {}) {
const { result, updateResult, addResolver } = resultContainer()
const { result, setValue, setError, addResolver } = resultContainer()
const hookProps = { current: initialProps }

const toRender = () => (
<TestHook callback={callback} hookProps={hookProps.current}>
{updateResult}
</TestHook>
<ErrorBoundary onError={setError}>
<Suspense fallback={<Fallback />}>
<TestHook callback={callback} hookProps={hookProps.current}>
{setValue}
</TestHook>
</Suspense>
</ErrorBoundary>
)

const { unmount, rerender: rerenderComponent } = render(toRender(), options)
Expand Down
51 changes: 51 additions & 0 deletions test/suspenseHook.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { renderHook, cleanup } from 'src'

describe('suspense hook tests', () => {
const cache = {}
const fetchName = (isSuccessful) => {
if (!cache.value) {
cache.value = new Promise((resolve, reject) => {
setTimeout(() => {
if (isSuccessful) {
resolve('Bob')
} else {
reject(new Error('Failed to fetch name'))
}
}, 50)
})
.then((value) => (cache.value = value))
.catch((e) => (cache.value = e))
}
return cache.value
}

const useFetchName = (isSuccessful = true) => {
const name = fetchName(isSuccessful)
if (typeof name.then === 'function' || name instanceof Error) {
throw name
}
return name
}

beforeEach(() => {
delete cache.value
})

afterEach(cleanup)

test('should allow rendering to be suspended', async () => {
const { result, waitForNextUpdate } = renderHook(() => useFetchName(true))

await waitForNextUpdate()

expect(result.current).toBe('Bob')
})

test('should set error if suspense promise rejects', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure this test actually adds much value. in #27 I discovered that a rejected Promise does not trigger the error boundary, and instead it's up to the suspending code to throw the error on the subsequent render (see line 24 for example), which means this is actually no different than if the hook just threw an error for some other reason (like we already do in the errorHook test suite.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyone who implements a Suspense library will be doing such so I think this provides value

Copy link
Member Author

@mpeyper mpeyper Apr 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, that was my thought too and why I kept it when cleaning up the commits.

const { result, waitForNextUpdate } = renderHook(() => useFetchName(false))

await waitForNextUpdate()

expect(result.error).toEqual(new Error('Failed to fetch name'))
})
})