Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

refactor: fetch add #1087

Merged
merged 18 commits into from
Sep 4, 2019
Merged

refactor: fetch add #1087

merged 18 commits into from
Sep 4, 2019

Conversation

alanshaw
Copy link
Contributor

@alanshaw alanshaw commented Sep 1, 2019

@alanshaw alanshaw changed the title refactor: fetch add [WIP] refactor: fetch add Sep 1, 2019
alanshaw added a commit to ipfs-inactive/interface-js-ipfs-core that referenced this pull request Sep 3, 2019
This PR updates the tests with new supported inputs for add.

refs ipfs-inactive/js-ipfs-http-client#1087

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@alanshaw alanshaw changed the title [WIP] refactor: fetch add refactor: fetch add Sep 3, 2019
alanshaw added a commit to ipfs-inactive/interface-js-ipfs-core that referenced this pull request Sep 3, 2019
* fix: supported add inputs

This PR updates the tests with new supported inputs for add.

refs ipfs-inactive/js-ipfs-http-client#1087

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>

* fix: test for hidden file - only 10 items

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>

* fix: test assertion

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>

* chore: appease linter

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>

* fix: test

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
addFromStream: require('../files-regular/add')(send),
_addAsyncIterator: require('../files-regular/add-async-iterator')(send),
add: (input, options, callback) => {
if (typeof options === 'function') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do these functions need to guard their inputs like this? I think the methods they chain to also guard. Maybe they could just be callbackify'd at this level?

src/add-from-fs/glob-source.js Outdated Show resolved Hide resolved
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
This will reduce as we switch more APIs to using fetch.

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@alanshaw alanshaw deleted the refactor/fetch-add2 branch September 4, 2019 11:22
@achingbrain
Copy link
Collaborator

achingbrain commented Sep 12, 2019

I think this might have broken IPFS on electron - it seems it doesn't support using FormData in it's fetch implementation to send files: electron/electron#9684

This is reflected in the logs for the electron-renderer tests.

ipfsd-ctl:daemon:stdout %c[1568271716653] INFO  (80746 on marwood.home): request completed
    req: {
      "id": "1568271716642:marwood.home:80746:k0gchk7y:10001",
      "method": "post",
      "url": "http://127.0.0.1:60676/api/v0/add?stream-channels=true",
      "headers": {
        "host": "127.0.0.1:60676",
        "connection": "keep-alive",
        "content-length": "17",
        "sec-fetch-mode": "cors",
        "origin": "file://",
        "user-agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/76.0.3809.146 Electron/6.0.8 Safari/537.36",
        "content-type": "text/plain;charset=UTF-8",
        "accept": "*/*",
        "sec-fetch-site": "cross-site",
        "accept-encoding": "gzip, deflate, br",
        "accept-language": "en-GB"
      }
    }
    res: {
      "statusCode": 200,
      "headers": {
        "x-chunked-output": "1",
        "content-type": "application/json; charset=utf-8",
        "trailer": "X-Stream-Error",
        "access-control-allow-headers": "X-Stream-Output, X-Chunked-Output, X-Content-Length",
        "access-control-expose-headers": "X-Stream-Output, X-Chunked-Output, X-Content-Length,WWW-Authenticate,Server-Authorization",
        "vary": "origin",
        "access-control-allow-origin": "file://",
        "cache-control": "no-cache",
        "connection": "close"
      }
    }
    responseTime: 11%c +610ms

Note how the outgoing content type is "text/plain;charset=UTF-8" and not multipart/form-data;boundary=----blahblah. The request body that's sent here is the string "[object FormData]" 😢

@hugomrdias
Copy link
Contributor

that's an old issue, how can i repro this? i wanna try to upgrade electron in aegir and test it

@achingbrain
Copy link
Collaborator

Run the Electron tests with DEBUG=ipfsd-ctl:* npx aegir test -t electron-renderer -- --bail --timeout 10000, the test

bitswap
       transfer a file between
         2 peers:

fails when it tries to add a file due to the request mangling above.

@alanshaw
Copy link
Contributor Author

To summary the problem:

  1. Electron renderer does not respect browser field in package.json
  2. ky-universal only polyfills, so existing native fetch is not replaced with node-fetch
  3. form-data (i.e. not native FormData) is passed to native fetch and gets stringified because it is not native FormData - i.e. native fetch does not know what it is

Workaround right now is to use the built browser bundle from a CDN or copied directly into the project.

@alanshaw
Copy link
Contributor Author

alanshaw commented Sep 12, 2019

Also, this could have been caught if the electron-renderer CI tests weren't allowed to fail.

See #1031

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