-
Notifications
You must be signed in to change notification settings - Fork 119
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: put files (client) #12
Conversation
46f7b98
to
3e5bb9d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking nice! Some minor tweaks suggested inline
const data2 = 'Hello web3.storage!!' | ||
|
||
return [ | ||
Web3File.fromBytes( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we Web3File.fromText()
web3-storage/web3-file#5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yey, just need to get it merged+shipped web3-storage/web3-file#4 to update here
packages/client/test/put.spec.js
Outdated
}) | ||
|
||
it('erros with a File that will not be parsed by the Cluster', async function () { | ||
this.timeout(35e10) // This needs to happen because of retry... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps add timeout and retry as configurable options to put?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to options as suggested, with default value from constant
Co-authored-by: Oli Evans <[email protected]>
packages/client/src/lib.js
Outdated
} | ||
|
||
// Destroy Blockstore | ||
await blockstore.destroy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be in a finally so always gets cleaned even if error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I found a bug in FsBlockstore that needs to get it to not break CI here: storacha/ipfs-car#49
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
*/ | ||
store(blob) { | ||
return Web3Storage.store(this, blob) | ||
put(files, options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are we going to expose the CID to the user before upload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking on an option:
onCarCreated(cid: CID)
, but I am leaving this to follow up PR to discuss
Co-authored-by: Alan Shaw <[email protected]>
Web3File.fromText( | ||
data2, | ||
'data2.zip', | ||
{ path: '/dir/otherdir/data2.zip' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use .txt so that they render properly if people ever open them on the gateway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Bear in mind that examples are still a WIP, I want to follow up on them with:
- docs
- integrate get
- add tests 🤖
"serve": "vite preview" | ||
}, | ||
"devDependencies": { | ||
"vite": "^2.3.7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the alternative is using skypack which is problematic by storacha/ipfs-car#27
I need to book some time next week to try to figure out an work around for that
fix Fixing the root redirect & Updating readme
This PR adds:
Needs: