TypeError: Cannot destructure property 'error' of 'results[(results.length - 1)]' as it is undefined. [Suspense]

[ntucker]

• react-hooks-testing-library version: 5.0.2
• react version: 17.0.1
• react-dom version (if applicable): 17.0.1
• react-test-renderer version (if applicable):17.0.1
• node version:14.13.1
• npm (or yarn) version:1.22.5

Relevant code or config:

expect(result.error).toBe(null);

What you did:

Directly after the renderHooks() line with a function that throws a Promise

What happened:

Reproduction:

result.error just after a hook that suspends (don't await the resolution of suspense).

result.current was fixed in dc21e59 but result.error was not.

Problem description:

Suspense is the most important use of this library

Suggested solution:

Do the thing in dc21e59 but apply to all members

[ntucker]

If something does not suspense is it still undefined? If so this is a really bad change as now one cannot test for suspense.

[ntucker]

Please tell me you set current to some other value if it does not suspend.

[mpeyper]

Hi @ntucker,

You're correct that the same fix should have been applied to error and was missed. My apologies for that.

With respect to you other points, I think you are misunderstanding the role of the results array and what current would be in a standard, non-suspense call.

I'll get a patch out for error (unless someone beats me to it) and leave a more comprehensive comment for the other points once my kids are in bed (about 2 hours from now).

[mpeyper]

Fix has gone out in version 5.0.3.

I have also updated the release notes of version 5.0.0 with a Breaking Change note about the change in functionality around both result.current and result.error (despite result.error not actually being usable until now).

[mpeyper]

Ok, now to your other points:

The changes were undocumented in 3.6 as they were never meant to affect the functionality of suspending hooks (i.e. it was bug of that version not a breaking change intended to make it version 4.0). The tests that would have caught that break only went out with 5.0 when I identified the issue with result.current and put the fix in for it, with a test to make sure it does not break again. I missed result.error at the time by accident as it was a minor fix during a larger feature build and was not my priority in the moment. Perhaps I should have come back and check more in the area, but I did not and can't change history on that one.

"Why was there no test to catch it prior to 3.6?" I hear you ask. Well, prior to 3.6, there was no special handling required to cater for a hook that had not returned a result (or error) in the first pass. In fact, I'd go so far as to say that "seeing" null in that circumstance was unintended behaviour from when suspense functionality was first added and the fact that it did not throw and returned something made it go unnoticed for well over a year. This probably isn't helped by the fact that many people use null and undefined more or less interchangeably to mean "no value"

The reason I say that null results were unintended is that when result.current or result.error get set, the other is set to undefined (commit before result.all changes were made), meaning a non-suspending renderHook call would never see the null as by the time it had returned the would either have a value or be replaced with undefined. This is likely also why my fix in 5.0 had these values returning undefined instead of the original null value. You must also remember that a non-trivial amount of time had passed since the original bug and the fix, so much of the context had been lost by then.

I actually believe undefined is the more correct value to use for a value that has never been set, so it's for this reason that I've decided to document the break and continue with it for version 5 onwards.

So in summary, when the changes were made in 3.6, I believed they were non-breaking as for a non-suspending hook call, the values are effectively internal variables and never get exposed. Obviously I failed to consider the suspending hook cases and a hole in our test suite (I'm aware your test suite did catch the bug) let it through unnoticed. I am sorry about that.

I hope that addresses your concerns about non-suspending hooks, the lack of documented breaks and the reasons it broke in the first place. If I have missed anything, please leave a comment and I will do my best to give you address it as well.

Finally, I want to address one final thing:

Suspense is the most important use of this library

I think you may be conflating your most import use of this library and everyones most important use of this library. Handling suspending hooks is something we support and not something we ever purposely break, but it's only one part of what this library offers. I honestly couldn't tell you what the most important feature we offer is, other than perhaps simply users being able to call a hook without defining a component themselves, as we have many users, each with their own most important use. In fact, until 5.0 we'd been having many people calling out for the ability to test their hooks with server rendering which does not support suspense at all.

I guess what I'm trying to say is that comments like these leave me very disheartened with the amount of care and effort I put into supporting all of our users the best way we can. I'm genuinely sorry that the break happened and has caused you the headaches it has. I'm also genuinely appreciative of the effort you put into identifying what the issue was and when it broke.

[mpeyper]

Note: I've also updated the release notes for version 3.6. I was still learning with semantic-release and had messed it up. I did not realise the fix did not capture the commit in the release notes (feat was in the commit description and not the summary so the commit was ignored).

I'll close this now as the issue has been resolved but please feel free to continue the discussion.

[ntucker]

Thanks for the quick response, and explanation!

I can see how undefined makes sense, tho the slight disadvantage here is that functions that don't return anything (thus really easy to happen) will return undefined, and thus in those cases suspense will be indistinguishable from when they return. On the other hand it would be clear that it is the expectation, vs null being somewhat arbitrary and still having (although less likely) a potential conflict with a real return value. Perhaps it might be better to have an additional value explicitly for suspense, but I think this is fine for now - especially since there are tests.

Library usage of suspense and other hooks

I can definitely see - at least in the current world - other hooks cases having utility without suspense. And obviously there's really no objective way to measure 'total impact' worldwide easily or accurately. I think for me having done both cases, the unique thing about Suspense is how incredibly convoluted testing was before this library. Vs other hooks in components, while not ideal, were not this level of difficulty. So from my experience it was the value of the use case - given that you had it - rather than the likelihood of a given person having a use case.

Of course this is all sort of silly if both can be supported well. I was kind of surprised this didn't have good test coverage as an early adopter (#27) of the library for suspense use case I was under the impression at that time that it was a strong use case.

My concern for depending on the library

@rest-hooks/test uses @testing-library/react-hooks as a dependency. This is a testing util built on top of this testing util. Because of that it's not a matter of working to make tests work.

What I realized yesterday is that since December 7th, anyone installing my library for the first time would experience a completely broken library. npm or yarn would resolve the ^3.2.1 to 3.6.0, which is broken and they would experience those errors. My test suite currently only uses the library to run its tests, so locking with yarn means I would not notice when minor/patch releases absolutely break since it will stay on 3.2.1. This is me relying on semver in an external package.

Now I have a few choices. I can release a fix version where I get rid of ^. This would at least solve it for new users, but not users who had the existing version and shuffle their installs. So I'm stuck with several versions of my library that - when released - worked fine, but are now broken.

A better world would be where 3.6.1 was released with this fix, so the entire 3 line wasn't destroyed.

If this doesn't happen, I'm stuck in a situation where my trust in this library has let down the trust of my users. For the short term I can absolutely fix the version, which isn't ideal because any improvements won't be had, but at least I won't fear them breaking.

So that's the context of that comment - the thinking I have to do to not let down my users. My intention was not for you to read it, as it certainly sounds harsh. I do appreciate the efforts you have put into this library, which is why I originally came to mark it as a dependency.

I expect most people are simply adding this as a devdependency, and so these situations are no more than a minor annoyance.

But if you can release a fix to the 3 line, I would very much appreciate it. Thanks again for the fix to the 5 line.

[ntucker]

(Although any fix to 3 line would also need null so it doesn't break)

[mpeyper]

Yes, I can see the issue you face. For what it's worth, it only would affect people asserting the pre-resolved values which in my experience, most people don't do (they only assert after the wait resolves), and I'm assuming you haven't seen any issues about it in the past month and a bit, so perhaps the issue isn't as widespread in actual usage as you're implying. I don't discount the risk of potential issues though.

If it helps, fixing the version to ~3.5.0 for your current version and then upgrading to ^5.0.0 in a major release would be fairly safe. No further improvements are planned for the older version so the risk of missing out on something is minimal and upgrading to the latest major of your library is always an option to get the shiny new features.

That said, if it's still an issue for you, I'm happy to patch the older version for you to retain the null and apply it to the ^4.0.0 branch as well of that helps. It should be possible without breaking the non-suspense cases but I'll let you know if I run into any issues.

I think you might be wrestling with what it actually means to have a dependency that is exposed to your users as well. I don't know exactly how we are exposed through your library (I haven't looked through your docs/code), but I cannot guarantee we will never ship another bug. I wish I could, but I can't. There will always be an inherent risks when you don't own the code yourself and the best you can hope for is that the dependencies you do have are actively maintained and are quick to push out patches when bugs are identified.

If that risk is more than you're willing to accept, then perhaps forking and maintaining it yourself is the more appropriate action to take. You can always synchronise with the mainline when new versions are pushed out. Just be sure to PR any of your own fixes and features hack again for consideration 😉

[ntucker]

Any idea why reactive/data-client#497 is failing now? This only happens in particular cases when i'm looping over providers (and obviously since upgrade to 5)