Cleaner code #17

amaury1093 · 2018-09-13T13:27:35Z

All RpcObservables and FrequencyObservables take an options? argument, which contains a provider field. If not empty, that provider will be used.
If provider is not specified, then they will use the global provider, defined in api.ts.
If an RpcObservable or FrequencyObservable is used before the provider is defined, then an error is thrown.
Removed overview. I don't think it's super useful, and only creates complications.

coveralls · 2018-09-13T13:33:10Z

Pull Request Test Coverage Report for Build 157

169 of 179 (94.41%) changed or added relevant lines in 18 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+4.07%) to 46.589%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/light.js/src/utils/testHelpers/mockApi.ts	31	32	96.88%
packages/light.js/src/rpc/other/post.ts	9	10	90.0%
packages/light.js/src/rpc/other/makeContract.ts	4	12	33.33%

Totals
Change from base Build 118:	4.07%
Covered Lines:	838
Relevant Lines:	1799

💛 - Coveralls

axelchalon · 2018-09-14T14:34:18Z

packages/light.js/src/frequency/utils/createPubsubObservable.ts

  const [namespace, method] = pubsub.split('_');
+  const api = provider ? createApiFromProvider(provider) : getApi();



we need to memoize at this point based on pubsub and api

for example

const createPubsubObservable = <T>( pubsub: string, { provider }: FrequencyObservableOptions = {} ) => { const [namespace, method] = pubsub.split('_'); const api = provider ? createApiFromProvider(provider) : getApi(); return memoize(createPubsubObservableWithApi)(pubsub, api); };

and put the rest of the code in createPubsubObservableWithApi

axelchalon

LGTM, except memoization needs to be made later on. Currently, if we change the api with a second call to setApi(), the next calls to the FrequencyObservable$ or RpcObservable$ will return the memoized observable based on the old api.

axelchalon · 2018-09-14T14:42:55Z

packages/light.js/src/rpc/other/makeContract.ts

+  (address: Address, abiJson: any[], provider: any) => {
+    const api = provider ? createApiFromProvider(provider) : getApi();
+    return api.newContract(abiJson, address);
+  }, // use types from @parity/abi
  { length: 1 } // Only memoize by address


memoization needs to be done based on address & api (provider ? createApiFromProvider(provider) : getApi()), otherwise if we do setApi() getContract() setApi() getContract() the last getContract() will return the memoized return value from the first getContract() call with the old api

axelchalon · 2018-09-14T14:43:29Z

packages/light.js/src/rpc/other/makeContract.ts

-    const abi = new Abi(abiJson);
+  (address: Address, abiJson: any[], options: { provider?: any } = {}) => {
+    const { provider } = options;
+    const abi = new Abi(abiJson); // use types from @parity/abi


memoization needs to be done based on address and api (same as above)

so it's a bit more tricky here
we could have getContract require an api/provider to be passed as parameter; put const api = provider ? createApiFromProvider(provider) : getApi(); here (in makeContract) and at that point memoize based on address and api

axelchalon · 2018-09-14T15:05:19Z

packages/light.js/src/rpc/rpc.ts

+
+const rpc = { ...eth, ...net, ...parity, post$ };
+
+export default memoizeAll(rpc);


remove memoization here (see previous comments)

axelchalon · 2018-09-14T15:11:12Z

packages/light.js/src/rpc/utils/createRpc.ts

+  options: RpcObservableOptions = {}
+) => {
+  const { provider, withoutLoading } = options;
+  const api = provider ? createApiFromProvider(provider) : getApi();
  // rpc$ will hold the RpcObservable minus its metadata
  const rpc$: RpcObservableWithoutMetadata<Source, Out> = (...args: any[]) => {


add memoization based on api and the observable args

axelchalon · 2018-09-14T15:14:53Z

packages/light.js/src/rpc/utils/createRpc.spec.ts

+
+beforeAll(() => {
+  setApi(resolveApi());
+});



can add a test for should memoize based on arguments & api

setApi(resolveApi()) const a = createRpc({})()(); const a2 = createRpc({})()(); expect(a).toEqual(a2); setApi(resolveNewApi()); const b = createRpc({})()(); expect(a).not.toEqual(b);

setApi(resolveApi()) const a = createRpc({})()('a'); const b = createRpc({})()('b'); expect(a).not.toEqual(b);

same for frequency tests

amaury1093 · 2018-09-17T13:48:32Z

For all func() that did something with api inside its body, I added funcWithApi(), which is a pure version of func(), and correctly memoized.

Also, for simplicity's sake, I removed the withoutLoading option, end-devs need to use the withouLoading operator to remove loading state.

amaury1093 · 2018-09-17T13:55:56Z

packages/light.js/src/rpc/utils/createRpc.ts

+      // `args` is arguments object as accessible in memoized function
+      return `${args[0].name}${args[1].provider.id}${JSON.stringify(
+        Array.from(args).slice(2)
+      )}`;


This is the only place where I'm not so satisfied.

Normalizer is a function that given the args, generates a unique id used for memoization.
For args[0] = metadata, I take the name, that's alright.
For args[1] = api object, I use provider.id, but this is not guaranteed to be unique per provider (or is it?)
Then for the other args, I JSON.stringify them, that should be alright too.

I didn't find anything with memoizee to customize a compare function, where we'd use === for the api argument.

Seems reasonable to me, I don't have any better idea

medikoo/memoizee#99

axelchalon · 2018-09-17T15:15:19Z

packages/light.js/src/rpc/other/makeContract.ts

-    return api.newContract(abiJson, address);
-  }, // use types from @parity/abi
+  (address: Address, abiJson: any[], api: any) =>
+    api.newContract(abiJson, address), // use types from @parity/abi
  { length: 1 } // Only memoize by address


does the memoization here needs to be based on api also or not?

Yes, api.newContract(...).contractMethod.call() makes a rpc call with that api object, fixed.

packages/light.js/src/rpc/utils/createRpc.ts

axelchalon

Awesome! 🍻

axelchalon · 2018-09-18T13:33:36Z

packages/light.js/src/rpc/utils/createRpc.ts

-    pipes.push(multicast(() => subject$), refCount());
-    if (options.withoutLoading === true) {
-      pipes.push(withoutLoading());
+    pipes.push(multicast(() => subject$), refCount(), distinctValues());


(we can probably use distinctReplayRefCount like in createPubsubObservable here)

Totally correct.

I used to put the subject$ inside the RpcObservable's metadata, so we can track the number of subscribers, the current value etc, but removed all that.

amaury1093 added 10 commits September 13, 2018 15:20

FrequencyObservable as a function returning a observable

8a59985

Fix tests

f113991

Test a new api

5e613d4

Make frequency observables work

9ecae21

Fix tests

660c5ef

Remove overview

6e99b67

Don't cover index.ts

e4298fa

Fix a lot of stuff

bb5509c

Remove useless import

1008d5a

Change testRegex for jest

3aa360b

amaury1093 added the package:light.js label Sep 13, 2018

amaury1093 requested a review from axelchalon September 13, 2018 13:27

amaury1093 changed the title ~~Am null provider~~ Cleaner code Sep 13, 2018

amaury1093 added 2 commits September 13, 2018 15:30

Add ambient for packages with no typigns

ea75324

Generate docs

7c03c5c

amaury1093 changed the title ~~Cleaner code~~ [WIP] Cleaner code Sep 13, 2018

amaury1093 added 4 commits September 13, 2018 16:37

Remove NullProvider

80b283f

Remove NullProvider

a46b19a

Regenerate docs

3447e95

Silent jest on CI

1f0c25a

amaury1093 changed the title ~~[WIP] Cleaner code~~ Cleaner code Sep 13, 2018

amaury1093 changed the title ~~Cleaner code~~ [WIP] Cleaner code Sep 13, 2018

amaury1093 added 6 commits September 13, 2018 17:18

Fix makeContract and post

b4404e2

Regen docs

165df16

Update summary

9b8edd8

Update ambient

fb89a94

Export post$ too

439d3bd

Fix makeContract

18e640f

amaury1093 changed the title ~~[WIP] Cleaner code~~ Cleaner code Sep 13, 2018

axelchalon reviewed Sep 14, 2018

View reviewed changes

amaury1093 added 6 commits September 17, 2018 11:16

Fix memoization for frequency observables

431fe51

Don't lint lib

44dde4a

Fix bugs with rpc$

15caa62

Remove useless packages

0cbe22a

Generate docs

85f8fb9

Update withoutLoading syntax

c5e0c56

amaury1093 added 2 commits September 17, 2018 15:52

Use json.stringify for normalizer

82fca45

Fix bug normalizer

4893e97

amaury1093 commented Sep 17, 2018

View reviewed changes

amaury1093 added 3 commits September 17, 2018 15:58

Remove withApi in docs

e449a57

Fix bug memoization

3a885fe

Remove onEvery2Blocks

c6ee32e

axelchalon reviewed Sep 17, 2018

View reviewed changes

packages/light.js/src/rpc/utils/createRpc.ts Show resolved Hide resolved

amaury1093 added 3 commits September 17, 2018 17:27

Options then args

041441b

CreateRpc in makeContract fix

8a066db

Fix getContract memoization

d2bb3c7

axelchalon reviewed Sep 17, 2018

View reviewed changes

packages/light.js/src/rpc/utils/createRpc.ts Outdated Show resolved Hide resolved

axelchalon approved these changes Sep 17, 2018

View reviewed changes

amaury1093 merged commit aab3ee3 into master Sep 17, 2018

amaury1093 deleted the am-null-provider branch September 17, 2018 16:13

axelchalon reviewed Sep 18, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleaner code #17

Cleaner code #17

amaury1093 commented Sep 13, 2018 •

edited

coveralls commented Sep 13, 2018 •

edited

axelchalon Sep 14, 2018

axelchalon Sep 14, 2018

axelchalon left a comment •

edited

axelchalon Sep 14, 2018

axelchalon Sep 14, 2018

axelchalon Sep 14, 2018 •

edited

axelchalon Sep 14, 2018

axelchalon Sep 14, 2018

axelchalon Sep 14, 2018 •

edited

amaury1093 commented Sep 17, 2018 •

edited

amaury1093 Sep 17, 2018

axelchalon Sep 17, 2018

amaury1093 Sep 17, 2018

axelchalon Sep 17, 2018

amaury1093 Sep 17, 2018 •

edited

axelchalon left a comment

axelchalon Sep 18, 2018

amaury1093 Sep 18, 2018

		const [namespace, method] = pubsub.split('_');
		const api = provider ? createApiFromProvider(provider) : getApi();


		const rpc = { ...eth, ...net, ...parity, post$ };

		export default memoizeAll(rpc);

Cleaner code #17

Cleaner code #17

Conversation

amaury1093 commented Sep 13, 2018 • edited

coveralls commented Sep 13, 2018 • edited

Pull Request Test Coverage Report for Build 157

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axelchalon left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axelchalon Sep 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axelchalon Sep 14, 2018 • edited

Choose a reason for hiding this comment

amaury1093 commented Sep 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 Sep 17, 2018 • edited

Choose a reason for hiding this comment

axelchalon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 commented Sep 13, 2018 •

edited

coveralls commented Sep 13, 2018 •

edited

axelchalon left a comment •

edited

axelchalon Sep 14, 2018 •

edited

axelchalon Sep 14, 2018 •

edited

amaury1093 commented Sep 17, 2018 •

edited

amaury1093 Sep 17, 2018 •

edited