feat: new store method that uploads token with all the assets and metadata - #56
Conversation
Deploying with
|
| Latest commit: |
229deed
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://87b1104e.nft-storage.pages.dev |
alanshaw
left a comment
alanshaw
left a comment
There was a problem hiding this comment.
This is awesome work 🚀
I think that where we have Token in this code we should replace with Metadata (or similar) so we don't confuse with our API key token.
Please could we get a really good example added to the README.md in place of what's currently there? 🙏
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
Codecov Report
@@ Coverage Diff @@
## main #56 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 2 3 +1
Lines 243 574 +331
==========================================
+ Hits 243 574 +331
Continue to review full report at Codecov.
|
b1dfd93 to
bee00e5
Compare
|
I have updated pull request so it now uses cluster and it is ready to be reviewed. |
alanshaw
left a comment
alanshaw
left a comment
There was a problem hiding this comment.
This looks incredible! A few things that can be sent as followup PRs if you like:
- We need an update to the OpenAPI schema
- We should alter the docs for the JS client on the homepage to use the new
storemethod - We should change the example here to use the new API
- We should add example(s?) here
| @@ -1,5 +1,6 @@ | |||
| import { Cluster } from '@nftstorage/ipfs-cluster' | |||
| import { cluster } from './constants.js' | |||
| import { CID } from 'multiformats' | |||
There was a problem hiding this comment.
can we just use the cid string ?
There was a problem hiding this comment.
We need CIDs to assemble an IPLD node. I could move the the CID string parsing to the other module, if that is what you want but CID is needed one way or the other.
@hugomrdias please let me know if this is ok, or if you want me to move the parsing to other module.
There was a problem hiding this comment.
i would prefer to remove this here but its not a blocker
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
…orage into feat/nft-meta-block
| "esModuleInterop": true, | ||
| // Need to disable this because generated typedefs omit generics which | ||
| // creates a problew for multiformats | ||
| // @see https://github.com/multiformats/js-multiformats#typescript-support |
There was a problem hiding this comment.
but we are on 4.2
Generated types don't have type parameters possibly because multiformats is either on older version of TS or maybe that bug has not been fixed yet.
There was a problem hiding this comment.
Oh 🤣 I just realised you meant typescript 4.2 - sorry ignore me
There was a problem hiding this comment.
I looked into this and seems that problem is not with multiformats, but rather with dag-cbor.
| @@ -1,5 +1,6 @@ | |||
| import { Cluster } from '@nftstorage/ipfs-cluster' | |||
| import { cluster } from './constants.js' | |||
| import { CID } from 'multiformats' | |||
There was a problem hiding this comment.
i would prefer to remove this here but its not a blocker
hugomrdias
left a comment
hugomrdias
left a comment
There was a problem hiding this comment.
LGTM, but i'm getting errors running api ts check
did you enable the git hooks like this https://github.com/ipfs-shipyard/nft.storage#development ?
i added an issue to add a github action to run api tests in the CI to avoid these problems in the future
|
I upgraded to multiformats@8 - the same version |
6 participants
Supersedes #50