Observables should not fire twice the exact same value

[amaury1093]

Repro:

      api.derive.staking.info(stashId, (result) => {
        ++count;

        console.error('***', count, JSON.stringify(result));
      }).catch(console.error);

Expected:

 *** 7 {"accountId":"5GNJqTPyNqANBkUVMN1LPPrxXnFouWXoe2wNSmmEoLctxiZY","controllerId":"5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY","nextSessionId":"5FA9nQDVg267DEd8m1ZypXLBnvN7SFxYwV7ndqSYGiN9TTpu","nominators":[],"redeemable":"0","rewardDestination":1,"stakers":{"total":"0x0000000000000000002386f26fc10000","own":"0x0000000000000000002386f26fc10000","others":[]},"stakingLedger":{"stash":"5GNJqTPyNqANBkUVMN1LPPrxXnFouWXoe2wNSmmEoLctxiZY","total":"0x0000000000000000002386f26fc10000","active":"0x0000000000000000002386f26fc10000","unlocking":[]},"stashId":"5GNJqTPyNqANBkUVMN1LPPrxXnFouWXoe2wNSmmEoLctxiZY","validatorPrefs":{"unstakeThreshold":3,"validatorPayment":0}}

...

 *** 8 something slightly different

Actual:

    *** 7 {"accountId":"5GNJqTPyNqANBkUVMN1LPPrxXnFouWXoe2wNSmmEoLctxiZY","controllerId":"5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY","nextSessionId":"5FA9nQDVg267DEd8m1ZypXLBnvN7SFxYwV7ndqSYGiN9TTpu","nominators":[],"redeemable":"0","rewardDestination":1,"stakers":{"total":"0x0000000000000000002386f26fc10000","own":"0x0000000000000000002386f26fc10000","others":[]},"stakingLedger":{"stash":"5GNJqTPyNqANBkUVMN1LPPrxXnFouWXoe2wNSmmEoLctxiZY","total":"0x0000000000000000002386f26fc10000","active":"0x0000000000000000002386f26fc10000","unlocking":[]},"stashId":"5GNJqTPyNqANBkUVMN1LPPrxXnFouWXoe2wNSmmEoLctxiZY","validatorPrefs":{"unstakeThreshold":3,"validatorPayment":0}}

...

    *** 8 {"accountId":"5GNJqTPyNqANBkUVMN1LPPrxXnFouWXoe2wNSmmEoLctxiZY","controllerId":"5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY","nextSessionId":"5FA9nQDVg267DEd8m1ZypXLBnvN7SFxYwV7ndqSYGiN9TTpu","nominators":[],"redeemable":"0","rewardDestination":1,"stakers":{"total":"0x0000000000000000002386f26fc10000","own":"0x0000000000000000002386f26fc10000","others":[]},"stakingLedger":{"stash":"5GNJqTPyNqANBkUVMN1LPPrxXnFouWXoe2wNSmmEoLctxiZY","total":"0x0000000000000000002386f26fc10000","active":"0x0000000000000000002386f26fc10000","unlocking":[]},"stashId":"5GNJqTPyNqANBkUVMN1LPPrxXnFouWXoe2wNSmmEoLctxiZY","validatorPrefs":{"unstakeThreshold":3,"validatorPayment":0}}

Notes:

• I added a distinctUntilChanged in drr in api-derive, but right now it's a bit useless, because it only checks on === for different values, should check at least on Codec and Codec[]
• The 2nd thing is I'm not sure distinctUntilChanged on api-derive is useful, it should probably take place lower down the level, so that repeated values don't happen on api.query or api.rpc.state.subscribeStorage

[jacogr]

Agreed that it probably needs to dit one lower. Not 100% sure how the update above happens - does the RPC really return 2 distinct values that we decode to 1? Or it doesn't and we decide to make it so?

[amaury1093]

does the RPC really return 2 distinct values that we decode to 1

No idea tbh, will have to investigate a bit

[pmespresso]

A quick sanity check suggests the RPC is returning the same value but the drr is not correctly comparing the returned objects.

this wil fail

drr.spec.ts
  it('should not fire twice the same object', (done): void => {
    let count = 0;
    of({ a: 1 }, { a: 1 }).pipe(drr()).subscribe((): void => { ++count; });

    setTimeout((): void => {
      expect(count).toBe(1);
      done();
    }, 50);
  });

Checking for deep equality in the drr with something like lodash will make the above test pass:

// drr.ts
import { isEqual } from 'lodash';

export const drr = (): DrrResult => <T> (source$: Observable<T>): Observable<T> =>
  source$.pipe(
    catchError((error): Observable<never> => {
      l.error(error);

      throw error;
    }),
    distinctUntilChanged(isEqual),
    publishReplay(1),
    refCount()
  );

That being said, the whole distinctUntilChanged() could be piped right into createMethodSubscribe for the Codec output from formatOutput in RPCCore.

[amaury1093]

I don't think deep equality will work because we're dealing with classes here (not 100% sure though), but anyway a better way to check would be to use .eq() from Codec. We can assume that there are only Codec and Codec[] coming back from rpc responses.

createMethodSubscribe sounds like a good place to put distinctUntilChanged.

[jacogr]

Only Codec from RPC responses.

[amaury1093]

Hmm, e.g. when I subscribe api.rpc.state.subscribeStorage([0x..., 0x...]), I get a Codec[] in return.

https://github.com/polkadot-js/api/blob/451dbf9eb303e242cbc812ba49d1b0b581611e57/packages/rpc-core/src/index.ts#L271

[jacogr]

Dangit, I forgot about my fav call, you are right.

We mangle this thing "incorrectly" though since StorageChangeSet has a Vec<KeyValueOption> - and it actually also breaks since we should really be consistent. But then we get into the age-old "it is not really a codec anymore", like we have on derive.

But that is not a problem for this one.

[polkadot-js-bot]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue if you think you have a related problem or query.