cache registry reverses in local storage #4182

derhuerst · 2017-01-16T17:22:08Z

Part of #4096.

still to do:

store the cache per chain

jacogr · 2017-01-19T12:40:12Z

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

 import Contracts from '~/contracts';
 import subscribeToEvents from '~/util/subscribe-to-events';

 import registryABI from '~/contracts/abi/registry.json';

 import { setReverse, startCachingReverses } from './actions';

+const read = (chain) => {
+  const reverses = window.localStorage.getItem(`${chain}-registry-reverses`);


Would prefer using store (i.e. import store from 'store';)

Would prefer the @ngotchac introduced _parity:: prefix (still need to migrate old)

Would prefer :: seperators as per above convention

Would prefer <prefix>::<type>::<subtype>, i.e. _partity::reverse::${chain}

👍
2.-3. Is there a specific reason for this? the . notation seems to be a de-facto standard. I agree with prefixing with parity, but why _parity? Does that help somehow, except worse readability?

It is not meant to be read by humans. :)

I just want everything to align with our codebase de-facto standards we have in-place now. I don't think what we have is 100% perfect, but it also doesn't detract and is consistent at least in all the new code. A good step.

We already have a mix now of old that still needs to be migrated, I would not like to add another way of doing things and make things even worse.

logLevel as an exception is tolerated since we can actually direct people to change manually in case of bugs and in this case since there is manual interaction it makes the simpler format better.

jacogr · 2017-01-19T12:40:14Z

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

+const read = (chain) => {
+  const reverses = window.localStorage.getItem(`${chain}-registry-reverses`);
+  const lastBlock = window.localStorage.getItem(`${chain}-registry-reverses-last-block`);
+  if (!reverses || !lastBlock) {


OCD-related comment (plus I'm old) - some spacing between vars and action (i.e. blank line here) would be appreciated.

jacogr · 2017-01-19T12:40:17Z

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

+  const reverses = getReverses();
+  const lastBlock = getLastBlock();
+
+  window.localStorage.setItem(`${chain}-registry-reverses`, JSON.stringify(reverses));


Additional bonus for using store (as commented above) - no need to manually strigify

jacogr · 2017-01-19T12:40:22Z

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

+        break;
+      case 'setReverse':
+        write(
+          () => store.getState().nodeStatus.netChain,


Why don't we just pass the values in as opposed to passing in a function that retrieves the values?

Premature optimization. 😉 write gets called quite often. For all past & new reverse events.

jacogr · 2017-01-19T12:40:32Z

js/src/redux/providers/registry/reducer.js

@@ -24,7 +24,7 @@ export default (state = initialState, action) => {
      return state;
    }

-    return { ...state, reverse: {
+    return { reverse: {


Would prefer spacey blocks reverse on next line just aligning with what we have. (Plus each entry on it's own line)

jacogr · 2017-01-19T16:45:49Z

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

+const STORE_KEY = '_parity::reverses';
+
+const read = (chain) => {
+  const reverses = store.get(`${STORE_KEY}::${chain}::data`);


Would probably make sense to const-ify data & lastBlock since it is used in multiple places. (Not a crisis either way)

jacogr · 2017-01-19T16:45:51Z

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

+
+const read = (chain) => {
+  const reverses = store.get(`${STORE_KEY}::${chain}::data`);
+  const lastBlock = store.get(`${STORE_KEY}::${chain}::last-block`);


Prefer lastBlock to last-block. Rationale -

First in with the keys we already have in storage

Fits in with the fact that we do camelCase for identifiers & filenames globally

jacogr · 2017-01-19T16:45:53Z

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

+  if (!reverses || !lastBlock) {
+    return null;
+  }
+  return { reverses, lastBlock };


Empty line before return.

jacogr · 2017-01-19T16:45:59Z

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

@@ -91,6 +127,17 @@ export default (api) => (store) => {
          clearTimeout(timeout);
        }

+        write.flush();
+
+        break;


No need for blank line before.

jacogr · 2017-01-19T16:46:00Z

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

+        write.flush();
+
+        break;
+      case 'setReverse':


Blank line before.

jacogr · 2017-01-19T16:46:46Z

Couple of small style & naming niggles, otherwise looks ok.

derhuerst added 4 commits January 11, 2017 12:22

cache reverses in localStorage

702e7a2

merge master into jr-reverse-localstorage

9176416

bugfixes 🐛, linting fixes 👕

62f158a

registry caching: store last block

1c2adc3

derhuerst added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Jan 16, 2017

derhuerst mentioned this pull request Jan 16, 2017

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

Closed

2 tasks

derhuerst added 2 commits January 18, 2017 12:51

merge master into jr-reverse-localstorage

293372d

registry caching: store per chain

9eb9714

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 18, 2017

jacogr reviewed Jan 19, 2017

View reviewed changes

jacogr 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 19, 2017

derhuerst added 3 commits January 19, 2017 14:19

merge master into jr-reverse-localstorage

42740d6

localStorage -> store

1ff7552

code style ✨

b0bc0b4

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

jacogr reviewed Jan 19, 2017

View reviewed changes

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

write.flush();

break;

case 'setReverse':

Copy link

Contributor

jacogr Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blank line before.

code style 👕

d9073da

jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 20, 2017

jacogr merged commit df9110d into master Jan 20, 2017

derhuerst deleted the jr-reverse-localstorage branch January 26, 2017 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cache registry reverses in local storage #4182

cache registry reverses in local storage #4182

derhuerst commented Jan 16, 2017 •

edited

Loading

jacogr Jan 19, 2017

derhuerst Jan 19, 2017

jacogr Jan 19, 2017

jacogr Jan 19, 2017

jacogr Jan 19, 2017

jacogr Jan 19, 2017

derhuerst Jan 19, 2017

jacogr Jan 19, 2017

jacogr Jan 19, 2017

jacogr Jan 19, 2017

jacogr Jan 19, 2017

jacogr Jan 19, 2017

jacogr Jan 19, 2017

jacogr commented Jan 19, 2017

cache registry reverses in local storage #4182

cache registry reverses in local storage #4182

Conversation

derhuerst commented Jan 16, 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

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

jacogr commented Jan 19, 2017

derhuerst commented Jan 16, 2017 •

edited

Loading