-
Notifications
You must be signed in to change notification settings - Fork 1.2k
WIP: Modify response to add body in React Native and logging daemon requests #2874
WIP: Modify response to add body in React Native and logging daemon requests #2874
Conversation
| .DS_Store | ||
| .connect-deps-cache/ | ||
| .connect-deps.json | ||
| prettier.config.js |
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.
Any objections to these changes making it into the .gitignore?
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.
@hugomrdias @achingbrain Thoughts? Thanks!
| if (options.blockWriteConcurrency != null) searchParams.set('block-write-concurrency', options.blockWriteConcurrency) | ||
|
|
||
|
|
||
| const formData = await toFormData(input) |
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.
@hugomrdias @achingbrain Do you have any stylistic objections to building formData in a separate line like this?
| // Just for logging | ||
| for await (const chunk of entry.content) { | ||
| console.log({ chunk }) | ||
| } | ||
| // end of extra logging code |
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.
Don't merge until we remove this
| await consumeUntilAfter(stream, Buffer.from(boundary)) | ||
|
|
||
| for await (const chunk of stream) { | ||
| console.log('multipart chunk', chunk) |
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.
Copied and pasted the contents of the it-multipart package into this repo so that there is a record of the log statements that were added when testing IPFS in React Native
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.
There will be a linting error unless we add the deps of that package as deps for ipfs-multipart https://github.com/achingbrain/it/blob/42faa5aeb9de0e07c956d73020bd74fd08e3029d/packages/it-multipart/package.json#L17-L20
|
|
||
| const Content = require('@hapi/content') | ||
| const multipart = require('it-multipart') | ||
| const multipart = require('./it-multipart') |
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.
Context on this change https://github.com/ipfs/js-ipfs/pull/2874/files#r389970339
|
@hugomrdias @achingbrain I updated this PR with the upstream changes from Hugo's PR removing |
| searchParams: options | ||
| }) | ||
|
|
||
| for await (const chunk of toIterable(res.body)) { |
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.
@hugomrdias It's interesting that there are still some methods where this toIterable(res.body) pattern is still around.
| // TODO: Update streamToAsyncIterator with any chances in light of | ||
| // this feature branch | ||
| const { streamToAsyncIterator, ndjson } = require('../lib/core') |
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.
@hugomrdias Can you give a short description of the new approach with streamToAsyncIterator compared with toIterable? Thanks!
| // Note: It's interesting that subscribe | ||
| // keeps this ndjson(tranformation(res)) pattern although | ||
| // that's now long from other IPFS methods | ||
| readMessages(ndjson(streamToAsyncIterator(res)), { |
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.
@hugomrdias For parallelism, would it make sense to get rid of this ndjson(transformer(res)) pattern here too since it was removed for other IPFS methods in the ky removal PR?
|
Closing this in favor of #2813 |
Closes #2847
This is still a draft. There still is an open discussion about whether this change should land in this package or upstream in React Native itself.
This PR is an updated version of ipfs-inactive/js-ipfs-http-client#1224 now that the
ipfs-http-clientpackage has become part of thejs-ipfsmonorepo.