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: put files (client) #12

Merged
merged 8 commits into from
Jul 2, 2021
Merged

feat: put files (client) #12

merged 8 commits into from
Jul 2, 2021

Conversation

vasco-santos
Copy link
Contributor

@vasco-santos vasco-santos commented Jul 1, 2021

This PR adds:

  • add Web3Files API in the client
  • adds Node.js and Browser examples
    • They work on my local setup, but I flagged them as WIP until we get the end to end thing

Needs:

@vasco-santos vasco-santos changed the title feat: add file in client feat: add file (client) Jul 1, 2021
@vasco-santos vasco-santos changed the title feat: add file (client) feat: put files (client) Jul 1, 2021
@vasco-santos vasco-santos marked this pull request as ready for review July 1, 2021 12:54
Copy link
Contributor

@olizilla olizilla left a 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(
Copy link
Contributor

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

Copy link
Contributor Author

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/examples/browser/main.js Outdated Show resolved Hide resolved
packages/client/examples/browser/main.js Outdated Show resolved Hide resolved
packages/client/examples/node.js/files.js Outdated Show resolved Hide resolved
packages/client/examples/node.js/package.json Outdated Show resolved Hide resolved
packages/client/examples/node.js/package.json Outdated Show resolved Hide resolved
packages/client/src/lib.js Outdated Show resolved Hide resolved
})

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...
Copy link
Contributor

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?

Copy link
Contributor Author

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 <oli@tableflip.io>
packages/client/examples/browser/index.html Outdated Show resolved Hide resolved
}

// Destroy Blockstore
await blockstore.destroy()
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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) {
Copy link
Member

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?

Copy link
Contributor Author

@vasco-santos vasco-santos Jul 1, 2021

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 <alan.shaw@protocol.ai>
Web3File.fromText(
data2,
'data2.zip',
{ path: '/dir/otherdir/data2.zip' }
Copy link
Member

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?

Copy link
Contributor Author

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"
Copy link
Member

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?

Copy link
Contributor Author

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

@olizilla olizilla merged commit 2c11f5e into main Jul 2, 2021
@olizilla olizilla deleted the feat/add-car branch July 2, 2021 08:08
orvn pushed a commit that referenced this pull request Dec 1, 2021
fix Fixing the root redirect & Updating readme
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.

3 participants