This commit fixes two bugs in the useDocuments hook, and adds corresponding tests.
1) The hook was leaking handlers and not cleaning up properly.
The useEffect teardown was trying to clean up listeners,
but the useEffect wasn't rerun when listeners were updated,
so it didn't actually work. Now we don't save listeners
in state, and instead we re-initialize them whenever the
documents list changes.
(I believe there may have been a related bug where
updates weren't being processed correctly, but I wasn't
able to get that to happen in unit tests; regardless,
I'm confident the new approach to listeners is better.)
2) The hook accepts document URLs or IDs in the list.
If URLs were passed in, updates wouldn't actually get
processed correctly because of how the internal state is
structured. Now that works.
There was also an issue where the dependency array to the useEffect was incomplete, which was working out fine in practice but seemed dangerous. I've restructured the hook to avoid that incomplete dependency array as well.
In addition to the 2 new unit tests which fail on old code and pass on new, I've also been using this modified hook in some app code which was broken by these bugs and the new version seems to be working well.
One note: I considered exporting a URL-keyed map instead of a DocumentId-keyed map, but that would be a breaking API change so I avoided that for now.
This commit fixes two bugs in the useDocuments hook, and adds corresponding tests.
1) The hook was leaking handlers and not cleaning up properly. The useEffect teardown was trying to clean up listeners, but the useEffect wasn't rerun when listeners were updated, so it didn't actually work. Now we don't save listeners in state, and instead we re-initialize them whenever the documents list changes.
(I believe there may have been a related bug where updates weren't being processed correctly, but I wasn't able to get that to happen in unit tests; regardless, I'm confident the new approach to listeners is better.)
2) The hook accepts document URLs or IDs in the list. If URLs were passed in, updates wouldn't actually get processed correctly because of how the internal state is structured. Now that works.
There was also an issue where the dependency array to the useEffect was incomplete, which was working out fine in practice but seemed dangerous. I've restructured the hook to avoid that incomplete dependency array as well.
In addition to the 2 new unit tests which fail on old code and pass on new, I've also been using this modified hook in some app code which was broken by these bugs and the new version seems to be working well.
One note: I considered exporting a URL-keyed map instead of a DocumentId-keyed map, but that would be a breaking API change so I avoided that for now.