Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

chore: converts remaining file api methods to async iterators #2517

Merged
merged 12 commits into from
Oct 23, 2019

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain force-pushed the add-remaining-file-async-iterator-methods branch from 67b9a27 to d9986ee Compare October 8, 2019 14:04
@@ -5,7 +5,7 @@ const all = require('async-iterator-all')
module.exports = function (self) {
// can't use callbackify because if `data` is a pull stream
// it thinks we are passing a callback. This is why we can't have nice things.
return (data, options, callback) => {
return function add (data, options, callback) {
Copy link
Member

Choose a reason for hiding this comment

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

Why move from arrow func to named func here?

Copy link
Member Author

Choose a reason for hiding this comment

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

To have function names in any stack traces emitted making them a teensy bit easier to read.

@@ -49,6 +49,7 @@ describe('files', function () {
const invalidPath = null
const stream = ipfs.getReadableStream(invalidPath)

stream.on('data', () => {})
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. Something changed for now it requiring to start pumping for the error to be emitted?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the old implementation, converting the pull stream to a readable stream calls s.sink(read) in pull-stream-to-stream which calls drain() on the next tick which causes data to start flowing, potentially before the consuming code has set up pipes or data event listeners.

I've opened pull-stream/pull-stream-to-stream#7 as this looks like a bug, at least, it's not consistent with the node docs.

The new implementation doesn't do this conversion so doesn't have the same bug.

Copy link
Member

Choose a reason for hiding this comment

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

good catch! 👍

lidel added a commit to ipfs/ipfs-companion that referenced this pull request Oct 14, 2019
This switches to async iterator version of ipfs.add
(introduced to js-ipfs in ipfs/js-ipfs#2517)
and ensures Node streams are replaced by deterministic version of readable-stream

Closes #757
lidel added a commit to ipfs/ipfs-companion that referenced this pull request Oct 15, 2019
* fix: /api/v0/add in Brave

This switches to async iterator version of ipfs.add
(introduced to js-ipfs in ipfs/js-ipfs#2517)
and ensures Node streams are replaced by deterministic version of readable-stream

Closes #757

* fix(cid): fast finish + allow osx to fail
Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

🥳 woohoo! Just a few feedback points but this look great!

src/core/components/files-regular/ls-async-iterator.js Outdated Show resolved Hide resolved
src/core/components/files-regular/ls-async-iterator.js Outdated Show resolved Hide resolved
src/core/components/files-regular/refs-async-iterator.js Outdated Show resolved Hide resolved
src/core/components/files-regular/refs-async-iterator.js Outdated Show resolved Hide resolved
})
})
})
.finally(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Wow, I did not realise promises had a finally...

Copy link
Member Author

Choose a reason for hiding this comment

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

Putting the Java into Javascript 😆

@alanshaw
Copy link
Member

ping @achingbrain 🙏

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants