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

docs(example): show folks how to use pull-streams instead #988

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@dignifiedquire
Member

dignifiedquire commented Sep 4, 2017

Always stream incoming data through pull streams and use browserify-aes directly from master now. I was able to add a 300Mb sized file with this quite quickly.

Fixes most issues in #952 I believe

@@ -62,7 +62,7 @@
</div>
<!-- The IPFS node module -->
<script src="//unpkg.com/ipfs/dist/index.min.js"></script>
<!-- <script src="//unpkg.com/ipfs/dist/index.min.js"></script> -->

This comment has been minimized.

@diasdavid

diasdavid Sep 4, 2017

Member

This was supposed to be the example that showed that "no extra batteries required". Kind of sad we lost that.

@diasdavid

diasdavid Sep 4, 2017

Member

This was supposed to be the example that showed that "no extra batteries required". Kind of sad we lost that.

This comment has been minimized.

@VictorBjelkholm

VictorBjelkholm Sep 4, 2017

Member

@diasdavid unpkg doesn't allow linking to a branch? We could also publish latest master ourselves on IPFS and link that instead

@VictorBjelkholm

VictorBjelkholm Sep 4, 2017

Member

@diasdavid unpkg doesn't allow linking to a branch? We could also publish latest master ourselves on IPFS and link that instead

This comment has been minimized.

@dignifiedquire

dignifiedquire Sep 4, 2017

Member

We don't have to keep this, with the new release that would be fine to use the unkpkg version again

@dignifiedquire

dignifiedquire Sep 4, 2017

Member

We don't have to keep this, with the new release that would be fine to use the unkpkg version again

path: file.name,
content: pullFilereader(file)
}]),
node.files.createAddPullStream(),

This comment has been minimized.

@diasdavid

diasdavid Sep 4, 2017

Member

This is awesome, but we shouldn't have examples with undocumented (and already agreed to change) API

Needs this solved first. ipfs/interface-ipfs-core#126

@diasdavid

diasdavid Sep 4, 2017

Member

This is awesome, but we shouldn't have examples with undocumented (and already agreed to change) API

Needs this solved first. ipfs/interface-ipfs-core#126

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Sep 4, 2017

Member

Awesome job @dignifiedquire! I'll keep this PR open until #988 (comment) is solved

Member

diasdavid commented Sep 4, 2017

Awesome job @dignifiedquire! I'll keep this PR open until #988 (comment) is solved

@VictorBjelkholm

This comment has been minimized.

Show comment
Hide comment
@VictorBjelkholm

VictorBjelkholm Sep 4, 2017

Member

Hm, a bit worried about this as most people haven't been exposed to pull-streams and we're supposed to support the API that most people use as well. This seems to amount to that even though you can use js-ipfs without using pull-streams, it's really inefficient and you should probably use pull-streams.

As this is supposed to be a simple example, I'm wondering if it's not best to stick with the non-pullstreams version.

Member

VictorBjelkholm commented Sep 4, 2017

Hm, a bit worried about this as most people haven't been exposed to pull-streams and we're supposed to support the API that most people use as well. This seems to amount to that even though you can use js-ipfs without using pull-streams, it's really inefficient and you should probably use pull-streams.

As this is supposed to be a simple example, I'm wondering if it's not best to stick with the non-pullstreams version.

@VictorBjelkholm

This comment has been minimized.

Show comment
Hide comment
@VictorBjelkholm

VictorBjelkholm Sep 4, 2017

Member

Maybe we could show it's done with non-pullstreams in comments (risk of having code that is not being run though, might not actually work...) or make the pullstream into a separate example for better performance. We don't want to alienate people who see a js-ipfs example for the first time.

Member

VictorBjelkholm commented Sep 4, 2017

Maybe we could show it's done with non-pullstreams in comments (risk of having code that is not being run though, might not actually work...) or make the pullstream into a separate example for better performance. We don't want to alienate people who see a js-ipfs example for the first time.

@dignifiedquire

This comment has been minimized.

Show comment
Hide comment
@dignifiedquire

dignifiedquire Sep 4, 2017

Member

We are currently alienating people with the browser crashing, so I think having pull-streams is much better than crashing and being slow. node streams are really not meant to be used in the browser in most situations I don't think it's good to show examples with them, rather use non streaming methods by default and educate users about better options

Member

dignifiedquire commented Sep 4, 2017

We are currently alienating people with the browser crashing, so I think having pull-streams is much better than crashing and being slow. node streams are really not meant to be used in the browser in most situations I don't think it's good to show examples with them, rather use non streaming methods by default and educate users about better options

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Sep 4, 2017

Member

We will have both API and once they are properly documented, we can use them in the examples. This is one of the priorities for 0.27 (and probably the target thing to get it released)

Member

diasdavid commented Sep 4, 2017

We will have both API and once they are properly documented, we can use them in the examples. This is one of the priorities for 0.27 (and probably the target thing to get it released)

docs(example): improve exchange example
Always stream incoming data through pull streams and use browserify-aes directly from master now. I was able to add a 300Mb sized file with this quite quickly.

@diasdavid diasdavid referenced this pull request Sep 11, 2017

Closed

⚡️ v0.26.0 RELEASE 🚀 #986

16 of 16 tasks complete

@diasdavid diasdavid added ready P0 - Critical and removed ready labels Oct 13, 2017

@diasdavid diasdavid added in progress and removed ready labels Oct 22, 2017

@diasdavid diasdavid changed the title from docs(example): improve exchange example to docs(example): show folks how to use pull-streams instead Oct 22, 2017

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Oct 30, 2017

Member

Actively working on ipfs/interface-ipfs-core#162 this week to then finish up this PR

Member

diasdavid commented Oct 30, 2017

Actively working on ipfs/interface-ipfs-core#162 this week to then finish up this PR

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Nov 3, 2017

Member

Who was interested on this PR, please review ipfs/interface-ipfs-core#162 (comment)

Member

diasdavid commented Nov 3, 2017

Who was interested on this PR, please review ipfs/interface-ipfs-core#162 (comment)

@diasdavid diasdavid self-assigned this Nov 6, 2017

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Nov 17, 2017

Member

Redirecting the efforts here to this PR #1086

Member

diasdavid commented Nov 17, 2017

Redirecting the efforts here to this PR #1086

@diasdavid diasdavid closed this Nov 17, 2017

@wafflebot wafflebot bot removed the in progress label Nov 17, 2017

@diasdavid diasdavid deleted the example/better-exchange branch Nov 17, 2017

@mitra42

This comment has been minimized.

Show comment
Hide comment
@mitra42

mitra42 Nov 17, 2017

@diasdavid - it would be useful to leave issues open until they are solved, even if the solution is elsewhere. I for one have been watching this thread in the hope of finding the docs implied by its title, " show folks how to use pull-streams instead " with the direction to #1086 , I can't find any answer to this question in that issue nor in ipfs/interface-ipfs-core#162, and I'm probably not the only one waiting on docs before converting app code to use pull-streams.

mitra42 commented Nov 17, 2017

@diasdavid - it would be useful to leave issues open until they are solved, even if the solution is elsewhere. I for one have been watching this thread in the hope of finding the docs implied by its title, " show folks how to use pull-streams instead " with the direction to #1086 , I can't find any answer to this question in that issue nor in ipfs/interface-ipfs-core#162, and I'm probably not the only one waiting on docs before converting app code to use pull-streams.

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Nov 17, 2017

Member

@mitra42 valid point. I tend to keep issues and PR open until they are fixed or converge elsewhere. It is hard to keep track of multiple issues discussing the same things and that is why I always leave a pointer saying that "x discussion continues at issue y".

@mitra42, for your case, ipfs/interface-ipfs-core#162 brought you something even better as you won't have to worry about pull-streams and just use ipfs.files.add with files and get hashes back. See the updated https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/FILES.md#add

See the updated example on #1086 that this PR was also updating (direct link here

function onDrop (event) {
onDragExit()
$errors.className = 'hidden'
event.preventDefault()
if (!node) {
return onError('IPFS must be started before files can be added')
}
const dt = event.dataTransfer
const filesDropped = dt.files
function readFileContents (file) {
return new Promise((resolve) => {
const reader = new window.FileReader()
reader.onload = (event) => resolve(event.target.result)
reader.readAsArrayBuffer(file)
})
}
for (let i = 0; i < filesDropped.length; i++) {
const file = filesDropped[i]
readFileContents(file)
.then((buffer) => {
node.files.add(Buffer.from(buffer), (err, filesAdded) => {
if (err) { return onError(err) }
const fl = filesAdded[0]
$multihashInput.value = fl.hash
$filesStatus.innerHTML = `Added ${file.name} as ${fl.hash}`
})
})
.catch(onError)
}
}
), as you will see there are no Readable or Pull Streams involved. I used this code files bigger than 750Mb, a significant improvement.

Member

diasdavid commented Nov 17, 2017

@mitra42 valid point. I tend to keep issues and PR open until they are fixed or converge elsewhere. It is hard to keep track of multiple issues discussing the same things and that is why I always leave a pointer saying that "x discussion continues at issue y".

@mitra42, for your case, ipfs/interface-ipfs-core#162 brought you something even better as you won't have to worry about pull-streams and just use ipfs.files.add with files and get hashes back. See the updated https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/FILES.md#add

See the updated example on #1086 that this PR was also updating (direct link here

function onDrop (event) {
onDragExit()
$errors.className = 'hidden'
event.preventDefault()
if (!node) {
return onError('IPFS must be started before files can be added')
}
const dt = event.dataTransfer
const filesDropped = dt.files
function readFileContents (file) {
return new Promise((resolve) => {
const reader = new window.FileReader()
reader.onload = (event) => resolve(event.target.result)
reader.readAsArrayBuffer(file)
})
}
for (let i = 0; i < filesDropped.length; i++) {
const file = filesDropped[i]
readFileContents(file)
.then((buffer) => {
node.files.add(Buffer.from(buffer), (err, filesAdded) => {
if (err) { return onError(err) }
const fl = filesAdded[0]
$multihashInput.value = fl.hash
$filesStatus.innerHTML = `Added ${file.name} as ${fl.hash}`
})
})
.catch(onError)
}
}
), as you will see there are no Readable or Pull Streams involved. I used this code files bigger than 750Mb, a significant improvement.

@mitra42

This comment has been minimized.

Show comment
Hide comment
@mitra42

mitra42 Nov 17, 2017

Email sent as this goes into multiple topics.

mitra42 commented Nov 17, 2017

Email sent as this goes into multiple topics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment