Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: client perma cache #127

Merged
merged 6 commits into from Jun 10, 2022
Merged

feat: client perma cache #127

merged 6 commits into from Jun 10, 2022

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented May 31, 2022

This PR adds perma-cache client to client package. There are potential improvements on the pipeline (like converting given URLs to nftstorage.link if they are IPFS urls), but keeping scope smaller for initial implementation.

It includes:

  • all 4 initial methods (put, delete, list, status)
    • Support for batching, retries, and rate limit control as needed
  • smoke tests with mock API (we can move on later to spin up the actual API + DB + Gateway)
  • workflow tests updated to run bundlesize and pre mocking
  • client docs separated between scopes, one (unchanged) Markdown file for gateway utils and other for perma cache docs.

Closes #92

BREAKING CHANGE: GET /perma-cache/status renamed to /perma-cache/account

@vasco-santos vasco-santos marked this pull request as ready for review May 31, 2022 09:22
@nftstorage nftstorage deleted a comment from cloudflare-pages bot May 31, 2022
packages/client/docs/perma-cache.md Outdated Show resolved Hide resolved
> Put multiple URLs into nftstorage.link perma-cache

```ts
put(urls: string[]): PromiseResult<PermaCacheEntry>[]>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
put(urls: string[]): PromiseResult<PermaCacheEntry>[]>
put(urls: string[]): Promise<PromiseResult<PermaCacheEntry>[]>

...although I'd be cautious exposing PromiseResult...You could make it a bit more erganomic by returning a custom type:

put(urls: string[]): Promise<CacheResult[]>

Where CacheResult is:

interface CacheSuccess {
  url: string
  size: number
  insertedAt: string
}

interface CacheFailure {
  url: string
  error: string
}

type CacheResult = CacheSuccess | CacheFailure

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think this is a good call 👍🏼

> Delete multiple URLs into nftstorage.link perma-cache

```ts
delete(urls: string[]): PromiseResult<PermaCacheDeletedEntry>[]>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
delete(urls: string[]): PromiseResult<PermaCacheDeletedEntry>[]>
delete(urls: string[]): Promise<PromiseResult<PermaCacheDeletedEntry>[]>

// value: {
// url: 'https://bafkreidyeivj7adnnac6ljvzj2e3rd5xdw3revw4da7mx2ckrstapoupoq.ipfs.nftstorage.link',
// success: true
// }
Copy link
Member

Choose a reason for hiding this comment

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

Same here:

delete(urls: string[]): Promise<CacheDeleteResult>

When is success false? It should be an error message no?

interface CacheDeleteSuccess {
  url: string
}

interface CacheDeleteFailure {
  url: string
  error: string
}

type CacheDeleteResult = CacheDeleteSuccess | CacheDeleteFailure

packages/client/docs/perma-cache.md Outdated Show resolved Hide resolved
> Get perma-cache account status

```ts
status(): omise<PermaCacheStatus>
Copy link
Member

Choose a reason for hiding this comment

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

Since put, delete and list are top level I would have assumed status was relating to a cached item - i.e. for getting the cache status of a given URL, not getting account related information.

Suggested change
status(): omise<PermaCacheStatus>
accountInfo(): Promise<AccountInfo>

The endpoint should maybe be /account instead also?

packages/client/src/lib/interface.ts Outdated Show resolved Hide resolved
packages/client/src/lib/interface.ts Outdated Show resolved Hide resolved
@cloudflare-pages
Copy link

cloudflare-pages bot commented Jun 7, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 57aaece
Status: ✅  Deploy successful!
Preview URL: https://5536a151.nftstorage-link.pages.dev
Branch Preview URL: https://feat-client-perma-cache.nftstorage-link.pages.dev

View logs

vasco-santos and others added 2 commits June 7, 2022 17:56
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
@vasco-santos vasco-santos force-pushed the feat/client-perma-cache branch 4 times, most recently from 6c6cf57 to 958c78d Compare June 7, 2022 17:13
BREAKING CHANGE: GET /perma-cache/status renamed to /perma-cache/account
@vasco-santos
Copy link
Member Author

@alanshaw renamed API route as you suggest, we are still in private beta. Thinking if we should keep /perma-cache/status for now as deprecated, or just get it out right away. What do you think?

@alanshaw
Copy link
Member

alanshaw commented Jun 7, 2022

@alanshaw renamed API route as you suggest, we are still in private beta. Thinking if we should keep /perma-cache/status for now as deprecated, or just get it out right away. What do you think?

It's the same response right? I'd maybe just stick a redirect in for now...

@vasco-santos vasco-santos requested review from alanshaw and removed request for alanshaw June 7, 2022 18:11
packages/client/docs/perma-cache.md Outdated Show resolved Hide resolved
packages/client/docs/perma-cache.md Outdated Show resolved Hide resolved
packages/client/docs/perma-cache.md Outdated Show resolved Hide resolved
packages/client/docs/perma-cache.md Outdated Show resolved Hide resolved
packages/client/docs/perma-cache.md Outdated Show resolved Hide resolved
Comment on lines 172 to 173
// @ts-ignore p-settle will always handle error internally
return cacheResults.map((cacheResult) => cacheResult.value)
Copy link
Member

Choose a reason for hiding this comment

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

So, I'd do the error handling in this map, then you lose a level of indentation above and I think don't need the @ts-ignore:

Suggested change
// @ts-ignore p-settle will always handle error internally
return cacheResults.map((cacheResult) => cacheResult.value)
return cacheResults.map((r, i) => {
return r.reason ? { url: urls[i], error: r.reason.message } : r.value
})

Comment on lines 151 to 154
return {
...res,
url, // decoded
}
Copy link
Member

Choose a reason for hiding this comment

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

url should be in the response no?

Suggested change
return {
...res,
url, // decoded
}
return res

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, the issue was in the mock 🤦🏼

packages/client/src/perma-cache.js Outdated Show resolved Hide resolved
packages/client/src/perma-cache.js Outdated Show resolved Hide resolved
packages/client/src/perma-cache.js Outdated Show resolved Hide resolved
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
endpoint
)
} else {
// @ts-ignore typescript complains about URL type cannot be undefined
Copy link
Member

Choose a reason for hiding this comment

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

Bah, maybe just while(true), move entry yielding above this conditional and break if !link?

@vasco-santos vasco-santos merged commit f426b08 into main Jun 10, 2022
@vasco-santos vasco-santos deleted the feat/client-perma-cache branch June 10, 2022 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SuperHot Client
2 participants