feat: andrel - 185 general refactors #187

andrehadianto · 2023-03-15T05:57:51Z

Description

This PR handles:

add cache control to getDropInformation with 5 seconds validity, store drop info and timestamp in local storage, compare timestamp and revalidate
Add comments to Buttons component to improve readability

Fixes #185

How to test

Buttons storybook

yarn run storybook
Go to localhost:3000

Add cache control:

Go to AllDrops.
Select any key in page 1, and go back to My Drops using the navigation button (to prevent reloading)

Screenshots/Screen Recording of Implementation

See comments

andrehadianto · 2023-03-15T05:59:25Z

Screen.Recording.2023-03-15.at.1.58.34.PM.mov

re-fetching getDrops

is only called when we run await getDrops(...)

cloudflare-workers-and-pages · 2023-03-15T05:59:28Z

Deploying with Cloudflare Pages

Latest commit:	`58e68c8`
Status:	✅ Deploy successful!
Preview URL:	https://7b51cb07.keypom-airfoil.pages.dev
Branch Preview URL:	https://andrel-185-general-refactors.keypom-airfoil.pages.dev

View logs

andrehadianto · 2023-03-15T07:26:29Z

storybook for buttons

tzeweiwee

Looks good overall but I'd like to suggest the drop store for the caching mechanism. Let me know your thoughts!

src/lib/keypom.ts

tzeweiwee · 2023-03-15T08:22:03Z

src/lib/keypom.ts

@@ -66,6 +66,16 @@ class KeypomJS {
    this.nearConnection = await connect(connectionConfig);
  };

+  dropInformationMeta: {


this object should be placed above constructor for consistency

Currently, we are only storing all drops. I think we should also store drop information (getDropInformation).

The current logic works, but I wanna extend the caching mechanism to Drop Manager as well. In addition, if we're caching the data here, I suggest we create something like a Redux store here to handle 2 scenarios.

get all drops, after rpc calling getDrops, we populate the ids OR only store properties required by AllDrops table (drop_id, metadata, next_key_id) in dropStore.paginatedDrops. Set expiry time for getDrops.

user clicks on a drop, in Drop Manager, page component will get the drop from the store

Suggestions:

// drop store structure const dropStore = { paginatedDrops: { // pageIndex => array of dropIds or partial drop object 1: ['dropid-1', 'dropid-2', ...], 2: ['dropid-11', 'dropid-12', ...], }, drops: { // dropId => dropData 'dropid-1': { rop_id: 'dropid-1', config: {}, ... }, 'dropid-2': { drop_id: 'dropid-2', config: {}, ... }, }, getDropsExpiryTime: 16334382, }; // in getDrops() // setting cache expiry time for getDrops const CACHE_MAX_AGE = 5000; // ms const newGetDropsExpiryTime = new Date(new Date().getTime() + CACHE_MAX_AGE).getTime(); // comparing current time and expiry time if (Date.now() > getDropsExpiryTime) { // getDropsExpired // reset store // fetch the drops again }

what do you think?

Also i prefer the expiry/max-age approach/terminologies, just referencing Cache control mechanism in http headers

agree with the terminologies. will make some changes to this. However, i'd stick with Date.now() compared to new Date().getTime() for performance reason 👍

src/lib/keypom.ts

src/App.tsx

tzeweiwee · 2023-03-16T07:48:54Z

src/lib/keypom.ts

+        this.dropStore.paginatedDrops[dropId] = {
+          ...(await this.getDropInfo({ dropId })),
+          getPageIndex: pageIndex,
+          getExpiryTime: newGetPaginatedDropsExpiryTime,


sorry my bad, can we remove get from the variable names. getXX sounds like a function name

tzeweiwee · 2023-03-16T07:55:34Z

src/lib/keypom.ts

+      const newGetDropsExpiryTime = new Date(Date.now() + CACHE_MAX_AGE).getTime();
+      this.dropStore.getDropsExpiryTime = newGetDropsExpiryTime;
+
+      this.dropStore.getDropsLastPage = start;


getDropsLastPage a bit confused by this, is it referencing the current last item index?

tzeweiwee · 2023-03-16T07:59:36Z

src/lib/keypom.ts

@@ -190,7 +207,21 @@ class KeypomJS {
    return null;
  };

-  getDrops = async ({ accountId, start, limit }) => await getDrops({ accountId, start, limit });
+  getDrops = async ({ accountId, start, limit }) => {


Suggested change

getDrops = async ({ accountId, start, limit }) => {

getDrops = async ({ accountId, start, limit }: { account: string; start: string | number; limit: number; }) => {

tzeweiwee · 2023-03-16T08:11:22Z

src/lib/keypom.ts

+    /** Get Drops caching logic */
+    if (
+      Date.now() > this.dropStore.getDropsExpiryTime ||
+      start !== this.dropStore.getDropsLastPage


i tested locally, if I paginate to next page (page 1 -> 2), then back to previous (2 -> 1), it's not showing the previous drops from store (1)

tzeweiwee · 2023-03-16T08:18:53Z

@andrehadianto works very well when i go to a drop manager from all drops page and back to all drops on page 1. But i think we should cache all paginated drops.

All scenarios before expiry time.
Scenario 1:

Go to all drops (page 1)
paginate to page 2
click previous button (page 1) - this is still calling getDrops

Scenario 2:

Go to all drops
paginate to page 2
click a drop, enter drop manager
user click back or all drops (breadcrumb)
user lands on all drops page 1 - this still calls getDrops

My suggestion would be to concatenate dropStore.drops or to restructure dropStore such that if user navigate to any page index, it will fetch the drops of that page index in store which is how the following structure came to mind at first

paginatedDrops: { 1: [drop1, drop2], 2: [drop11, drop12], ... }

tzeweiwee

Looks good! thanks for working on this

andrehadianto added 5 commits March 15, 2023 11:34

feat: implement button story

12e57e8

build: add yarn run storybook

f35ad6e

feat: add logic to only show storybook routes

56ae1c4

chore: more explanatory label

2fa3f73

feat: getDrops caching

b242c68

andrehadianto self-assigned this Mar 15, 2023

chore: remove console.log

dcd585b

andrehadianto changed the title ~~Andrel/185 general refactors~~ feat: andrel - 185 general refactors Mar 15, 2023

andrehadianto requested a review from tzeweiwee March 15, 2023 07:28

andrehadianto marked this pull request as ready for review March 15, 2023 07:29

tzeweiwee suggested changes Mar 15, 2023

View reviewed changes

andrehadianto added 3 commits March 16, 2023 11:34

chore: update comment

c117fb1

chore: use redux and cache control terminoloy

eb79dc3

feat: drop manager drop caching

63161d0

tzeweiwee reviewed Mar 16, 2023

View reviewed changes

tzeweiwee and others added 2 commits March 16, 2023 16:12

feat: change breadcrumblink to navigate on client side

ffa179c

chore: variable names

cca4f8b

andrehadianto added 3 commits March 16, 2023 16:26

feat: cache each page for GetDrops

f27a490

feat: paginated drops caching logic

872ac4b

chore: rename variables

58e68c8

andrehadianto requested a review from tzeweiwee March 17, 2023 08:08

tzeweiwee approved these changes Mar 17, 2023

View reviewed changes

andrehadianto merged commit 7cd9184 into testnet Mar 17, 2023

tzeweiwee deleted the andrel/185-general-refactors branch March 17, 2023 08:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: andrel - 185 general refactors #187

feat: andrel - 185 general refactors #187

andrehadianto commented Mar 15, 2023 •

edited

Loading

andrehadianto commented Mar 15, 2023

cloudflare-workers-and-pages bot commented Mar 15, 2023 •

edited

Loading

andrehadianto commented Mar 15, 2023

tzeweiwee left a comment

tzeweiwee Mar 15, 2023

tzeweiwee Mar 15, 2023

tzeweiwee Mar 15, 2023 •

edited

Loading

tzeweiwee Mar 15, 2023

andrehadianto Mar 16, 2023

tzeweiwee Mar 16, 2023

tzeweiwee Mar 16, 2023

tzeweiwee Mar 16, 2023

tzeweiwee Mar 16, 2023

tzeweiwee commented Mar 16, 2023 •

edited

Loading

tzeweiwee left a comment

	getDrops = async ({ accountId, start, limit }) => {
	getDrops = async ({ accountId, start, limit }: { account: string; start: string \| number; limit: number; }) => {

feat: andrel - 185 general refactors #187

feat: andrel - 185 general refactors #187

Conversation

andrehadianto commented Mar 15, 2023 • edited Loading

Description

How to test

Buttons storybook

Add cache control:

Screenshots/Screen Recording of Implementation

andrehadianto commented Mar 15, 2023

cloudflare-workers-and-pages bot commented Mar 15, 2023 • edited Loading

Deploying with Cloudflare Pages

andrehadianto commented Mar 15, 2023

tzeweiwee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzeweiwee Mar 15, 2023 • 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

tzeweiwee commented Mar 16, 2023 • edited Loading

tzeweiwee left a comment

Choose a reason for hiding this comment

andrehadianto commented Mar 15, 2023 •

edited

Loading

cloudflare-workers-and-pages bot commented Mar 15, 2023 •

edited

Loading

tzeweiwee Mar 15, 2023 •

edited

Loading

tzeweiwee commented Mar 16, 2023 •

edited

Loading