fix: bindFirestoreRef not reacting to getter

[ralacerda]

Closes #1495

Checking if the docOrCollectionRef is a ref before setting up a watch to re-bind the firestore meant getters weren't being watched.

Using watch(() => toValue(docOrCollectionRef), bindFirestoreRef) will keep the old behaviour of tracking refs and computeds while also tracking getters.

In this case, we also don't need to check what docOrCollectionRef is.
Unless there is a cost in watching an non-reactive source (I don't think so, but I'm not sure).

I'm still learning my way around the tests, I tried the following, but the tests hangs and fails:

it('can be bound to a getter', async () => {
  const listCollectionRef = collection<{ name: string }>()
  const listRef = ref<null | CollectionReference>(null)

  const { wrapper } = factory({
    ref: () => listRef.value,
  })

  await addDoc(listCollectionRef, { name: 'a' })
  await addDoc(listCollectionRef, { name: 'b' })

  expect(wrapper.vm.list).toHaveLength(0)
  listRef.value = listCollectionRef
  expect(wrapper.vm.list).toContainEqual({ name: 'a' })
  expect(wrapper.vm.list).toContainEqual({ name: 'b' })
})

But I must be doing something wrong because even when I use ref: listRef it also fails, but ref: listCollectionRef it works.
I'll keep the PR as a draft while I work on those tests.

[posva]

For the test: rather than using a ref of a collection, put a string in the ref and use () => collection(db, '...', myRef.value)

[posva]

Could you also add the same for useDatabaseRef.ts and storage.ts?

[ralacerda]

I returned the isRef checking but also included typeof === 'function' as you suggested.
I did the same for useDatabaseRef.ts and storage.ts as you asked.

I wrote a test for useDatabase that it's pretty similar to the one above it.

I was able to add a test for useDocument with your suggestion of using a ref for the collection name.

The current implementation uses an named collection (populatedCollection and emptyCollection) which I guess can be a problem if another test tries to use the same named collection.

Should I use collection() util to create an unique collection for this test first, and then, create a document and populatedCollection inside it? I wanted to ask because I'm not sure if it's really needed.

Something like:

it('can be bound to a getter', async () => {
  const parentCollectionName = collection().path.split('/').at(-1);
  const populatedCollection = collection<{ name: string }>(
    parentCollectionName,
    'parentDocument',
    'populatedCollection'
  );
  const collectionName = ref('populatedCollection');

  const { wrapper, promise } = factory({
    ref: () =>
      collection(parentCollectionName, 'parentDocument', collectionName.value),
  });

  await promise.value;
  await addDoc(populatedCollection, { name: 'a' });
  await addDoc(populatedCollection, { name: 'b' });

  expect(wrapper.vm.list).toHaveLength(2);
  expect(wrapper.vm.list).toContainEqual({ name: 'a' });
  expect(wrapper.vm.list).toContainEqual({ name: 'b' });

  collectionName.value = 'emptyCollection';
  await nextTick();
  await promise.value;
  await nextTick();
  expect(wrapper.vm.list).toHaveLength(0);
});

[posva]

It should be fine the way you did it it, no need to create a subcollection 🙂

[posva]

Thanks a lot!

[posva]

I published a patch 👍