cache registry reverses, completion in address selector #4066

derhuerst · 2017-01-06T12:38:51Z

Address entry selector should autocomplete to names in the Registry (this will require scouring logs and building a cache of all names that have been reverse-registered).

#3991

Users quickly want all reverses registered in the path, but don't care so much about live data.

coveralls · 2017-01-06T13:27:36Z

Changes Unknown when pulling ad3b89d on jr-reverse-caching into ** on master**.

gavofyork · 2017-01-06T15:06:30Z

test failing.

gavofyork · 2017-01-06T15:17:41Z

running on mainnet with gavofyork as a reverse-lookup name:

i expect to see it autocompleting

gavofyork · 2017-01-06T15:17:55Z

it would also be nice to get it listed in there.

derhuerst · 2017-01-06T15:33:14Z

running on mainnet with gavofyork as a reverse-lookup name:
i expect to see it autocompleting

it only autocompletes in the reverse direction. this is what reverse entries are for right?

forward (non-reverse) entries don't get autocompleted yet. you should be able to type gavofyork however to get the full address (if you registered gavofyork and set A of course).

derhuerst · 2017-01-06T17:45:52Z

btw: Travis reported a passing test even though Gitlab failed. @General-Beck?

derhuerst · 2017-01-06T18:07:44Z

forward (non-reverse) entries don't get autocompleted yet.

Now part of #3991.

coveralls · 2017-01-06T20:15:27Z

Changes Unknown when pulling ba4263e on jr-reverse-caching into ** on master**.

gavofyork · 2017-01-07T14:55:23Z

it only autocompletes in the reverse direction
wtf?!

the point is to autocomplete gavo -> gavofyork:

Address entry selector should autocomplete to names in the Registry (this will require scouring logs and building a cache of all names that have been reverse-registered).

let me know if there is something not clear about this statement.

coveralls · 2017-01-09T15:01:47Z

Coverage increased (+0.05%) to 86.565% when pulling 8c2aa4d on jr-reverse-caching into 4c94878 on master.

ngotchac · 2017-01-09T15:42:09Z

js/src/redux/providers/registry/middleware.js

+export default class RegistryMiddleware {
+  toMiddleware () {
+    return (store) => {
+      const api = Contracts.get()._api;


Might be better to add api passed as argument to the toMidleware method, or to the constructor.

Would actually like to introduce plain functions for middlewares. Having classes really doesn't give any benefit and the benefit of consistency doesn't outweigh the cost in terms of readability & complexity.

ngotchac · 2017-01-09T15:45:23Z

js/src/redux/providers/registry/middleware.js

+                confirmedEvents = subscribeToEvent(_contract, 'ReverseConfirmed');
+                confirmedEvents.on('log', onLog);
+
+                removedEvents = subscribeToEvent(_contract, 'ReverseRemoved');


Why not subscribe to both events in one filter ?

Not a real reason, except that subscribeToEvent uses contract.subscribe(eventName) under the hood, which is a nice abstraction over the low-level filter API.

Yep, but could be nice to be able to give an array of events then :) Even better, have one callback per event, and it would call the right one automagically

So would you change it to use newFilter internally? I guess it would still be limited to one address?

Yep, I think you should be able to pass an Array of events, and on the filter's callback you get the matching event signature anyway, so then it would call the right callback, right?

ngotchac · 2017-01-09T15:48:22Z

js/src/redux/providers/registry/middleware.js

+                removedEvents.on('log', onLog);
+
+                timeout = setTimeout(checkReverses, 5000);
+                interval = setInterval(checkReverses, 20000);


I think you should clear the timeouts and intervals before setting up new ones as a general statement. If they get overwritten even once, then you won't be able to clear them... Might want to use a debounce function though (which can be canceled), would be easier than managing those timers.

I think you should clear the timeouts and intervals before setting up new ones as a general statement.

Totally agree! Just didn't pay attention to the fact that startCachingReverses might be fired twice. 😛

Might want to use a debounce function though (which can be canceled), would be easier than managing those timers.

How would it be easier?

It's just to take into account a scenario where this gets called every 2 seconds let's say, thus clearing the timeouts and setting up a new one in 5 seconds, that would never get called

By the way, why waiting 5 seconds at first ?

I must admit this is kind of premature optimisation. It exists to reduce the load when the UI just loaded, as completion in the address selector is async anyway and far less important than say account balances.

Okay yes. Might be worth remembering this when working on the local storage cache issue :)

For reference: #4096.

ngotchac · 2017-01-09T15:52:37Z

js/src/ui/Form/AddressSelect/addressSelectStore.js

+        const addr = Object
+          .keys(this.reverse)
+          .find((addr) => {
+            if (addr.toLowerCase().slice(0, query.length) === query) {


new RegExp('^' + query, 'i').test(addr) ?

ngotchac · 2017-01-09T15:53:27Z

js/src/ui/Form/AddressSelect/addressSelectStore.js

+            }
+
+            const name = this.reverse[addr];
+            return name.toLowerCase().slice(0, query.length) === query;


name might be undefined right?

ngotchac · 2017-01-09T15:55:30Z

One question : why using a middleware here? (except to hook to the initAll call). Might be simpler with reducer + actions...

ngotchac · 2017-01-09T16:06:53Z

js/src/ui/Form/AddressSelect/addressSelectStore.js

@@ -194,6 +239,8 @@ export default class AddressSelectStore {
          .filter((result) => result && !ZERO.test(result.address));


Might actually want to get rid of this.registryValues = []; line 231, to prevent the flickering when typing an autocompleted name.

This is dangerous, especially when we have (or add in the future) long-running queries, as it will show incorrect values to the users for a while. I'd rather introduce a loading flag.

Hmm yes that's true. The flickering is a bit odd, but no biggies

ngotchac · 2017-01-09T16:09:20Z

js/src/redux/providers/registry/middleware.js

+              .instance
+              .reverse
+              .call({}, [ address ])
+              .then((reverse) => store.dispatch(setReverse(address, reverse)));


Might want to make sure that the reversed addressed actually changed ; Redux dispatches can be costly as they might re-render all connected Components.

Might want to make sure that the reversed addressed actually changed;

Agree, keeping the nr of actions low is a nice thing.

Redux dispatches can be costly as they might re-render all connected Components.

It is not the job of Redux to make React performant. I'd rather fix the riffing/update logic in our components.

Might be another solution to let the reducer return the state directly. A future, less premature optimisation, would then be to use immutable state trees, which check oldState === newState.

Immutables would be really nice actually, not only in Redux. We had some issues with data being modified where it should not have been... And the thing is that at each action dispatch, the mapStateToProps will get called. So even if this returns non-updating results, it would still be nice to keep these calls low.

In private development, I use seamless-immutable, which only Object.freezes in dev mode to stay fast in production mode.

That's an interesting idea ! Would be worth trying IMO

coveralls · 2017-01-09T18:59:04Z

Coverage increased (+0.06%) to 86.573% when pulling f12937a on jr-reverse-caching into 4c94878 on master.

ngotchac · 2017-01-10T10:19:07Z

js/src/ui/Form/AddressSelect/addressSelectStore.js

@@ -36,6 +36,7 @@ export default class AddressSelectStore {
      if (query.length === 0 || query === '0x') {
        return null;
      }
+      const startsWithQuery = (s) => new RegExp('^' + query, 'i').test(s);


ngotchac · 2017-01-10T10:35:27Z

Looks good to me, except the little flickering when typing a name. What about something like this:

diff --git a/js/src/ui/Form/AddressSelect/addressSelectStore.js b/js/src/ui/Form/AddressSelect/addressSelectStore.js
index 1c93088..50e02ba 100644
--- a/js/src/ui/Form/AddressSelect/addressSelectStore.js
+++ b/js/src/ui/Form/AddressSelect/addressSelectStore.js
@@ -228,10 +228,12 @@ export default class AddressSelectStore {
       });
 
     // Registries Lookup
-    this.registryValues = [];
-
     const lookups = this.regLookups.map((regLookup) => regLookup(value));
 
+    const timeoutId = window.setTimeout(() => {
+      this.registryValues = [];
+    }, 200);
+
     Promise
       .all(lookups)
       .then((results) => {
@@ -241,6 +243,8 @@ export default class AddressSelectStore {
       .then((results) => {
         results = uniqBy(results, (result) => result.address);
 
+        window.clearTimeout(timeoutId);
+
         this.registryValues = results
           .map((result) => {
             const lowercaseAddress = result.address.toLowerCase();

This would remove the values if the results are not fetched after 200ms.

derhuerst · 2017-01-10T11:03:35Z

@ngotchac I'm definitely in favour of fixing this flickering, but not in this PR. Created #4103.

derhuerst added 6 commits January 5, 2017 16:56

reverse caching: redux boilerplate

ac2c678

reverse caching: middleware

b38aa8d

address selector: use cached reverses

6f492a4

reverse caching: optimize timing

71e80f1

Users quickly want all reverses registered in the path, but don't care so much about live data.

address selector: simple completion for reversed addresses

2aa76e5

Merge master into jr-reverse-caching

ad3b89d

derhuerst added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Jan 6, 2017

gavofyork mentioned this pull request Jan 6, 2017

Address entry & display components should use Registry #3991

Closed

4 tasks

merge master into jr-reverse-caching

c4f7b8f

fix tests ✅

ba4263e

gavofyork added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 7, 2017

derhuerst added 4 commits January 9, 2017 12:50

merge master into jr-reverse-caching

92742e6

address selector: lower case for reverse completion

38734c9

address selector: complete reverses by name as well

f05ffd2

address selector: unique registry results

8c2aa4d

derhuerst added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jan 9, 2017

derhuerst mentioned this pull request Jan 9, 2017

address selector: cache forward & reverse entries in local storage #4096

Closed

2 tasks

ngotchac reviewed Jan 9, 2017

View reviewed changes

ngotchac added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 9, 2017

reverse caching: pass API into middleware

61a42cc

ngotchac reviewed Jan 9, 2017

View reviewed changes

derhuerst added 3 commits January 9, 2017 17:49

reverse caching: PR grumbles

cca7627

subscribeToEvent -> subscribeToEvents

67c2981

reverse caching: use subscribeToEvents

f12937a

ngotchac reviewed Jan 10, 2017

View reviewed changes

ngotchac added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jan 10, 2017

jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Jan 10, 2017

derhuerst merged commit 8958603 into master Jan 10, 2017

derhuerst deleted the jr-reverse-caching branch January 10, 2017 11:31

derhuerst mentioned this pull request Jan 10, 2017

basic tests for subscribeToEvents #4115

Merged

ngotchac mentioned this pull request May 12, 2017

Introduce Directory contract and integrate into address-entry autocomplete #3091

Closed

		@@ -194,6 +239,8 @@ export default class AddressSelectStore {
		.filter((result) => result && !ZERO.test(result.address));

cache registry reverses, completion in address selector #4066

cache registry reverses, completion in address selector #4066

Conversation

derhuerst commented Jan 6, 2017

coveralls commented Jan 6, 2017

gavofyork commented Jan 6, 2017

gavofyork commented Jan 6, 2017

gavofyork commented Jan 6, 2017

derhuerst commented Jan 6, 2017

derhuerst commented Jan 6, 2017

derhuerst commented Jan 6, 2017

coveralls commented Jan 6, 2017

gavofyork commented Jan 7, 2017

coveralls commented Jan 9, 2017

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

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

Choose a reason for hiding this comment

derhuerst Jan 9, 2017 • edited Loading

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

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

ngotchac commented Jan 9, 2017

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

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

Choose a reason for hiding this comment

coveralls commented Jan 9, 2017

Choose a reason for hiding this comment

ngotchac commented Jan 10, 2017

derhuerst commented Jan 10, 2017 • edited Loading

derhuerst Jan 9, 2017 •

edited

Loading

derhuerst commented Jan 10, 2017 •

edited

Loading