fix(afs): Allow multiple subscribers by using share, closes #1191

[jamesdaniels]

Checklist

• Issue number for this PR: Multiple subscribes to AFS Collections cause issues #1191
• Docs included?: n/a
• Test units included?: yes
• In a clean directory, npm install, npm run build, and npm test run successfully? yes

Description

Subscribing to AFS collection multiple times causes duplicate data to show up. This is due to events replaying in fromRef#combineChange.

Adding share() to collection#snapshotChange fixes this issue as the side-effects will only occur the once.

[davideast]

Great work, @jamesdaniels! Just a few things :)

[davideast]

Nest these subscriptions to test that one happens after another. I did this locally:

it('should handle multiple subscriptions', async (done) => {
      const ITEMS = 4;
      const { randomCollectionName, ref, stocks, names } = await collectionHarness(afs, ITEMS);
      const obs = stocks.valueChanges();
      const sub1 = obs.subscribe(values1 => {
        const sub2 = obs.subscribe(values2 => {
          expect(values1.length).toEqual(values2.length);
          deleteThemAll(names, ref).then(done).catch(done.fail);
        });
      });
    });

[jamesdaniels]

Ah good point.

[davideast]

We need the .share() operator further down in fromRef.ts. That way each observable method benefits from a "shared" source.

export function fromRef<R>(ref: firebase.firestore.DocumentReference | firebase.firestore.Query) {
  return _fromRef<typeof ref, R>(ref).share();
}

[jamesdaniels]

SGTM, but as a optimization.
As mentioned, we need the share on sortedChanges (which I'll bump up to the definition) to make the side effects (combineChanges) occur only the once.

[jamesdaniels]

@davideast found a couple more hairy problems here, spent the evening hacking work arounds together but couldn't find anything more elegant then firing next(undefined) before onSnapshot to indicate a reset. It might not be pretty but it will solve the problem cases I defined in the tests.

[davideast]

I think we need .share() since .shareReplay() caches the data.

[jamesdaniels]

One of my fixes introduced in 28aef60 (passing undefined) no longer works against master, since we are no longer filtering out null sets (eb71edc). I made that test pending while I think on a new, hopefully more elegant, solution.

This should handle most cases though.

[jamesdaniels]

Ok I found the troubles, another commit coming in shortly.