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

chore: reference only local files in package.json "browser" field #3630

Closed
wants to merge 4 commits into from

Conversation

yurtsiv
Copy link

@yurtsiv yurtsiv commented Apr 20, 2021

Fixes this issue: #3557

Related PR in js-ipfs-utils: ipfs/js-ipfs-utils#121

@welcome

This comment has been minimized.

@@ -18,9 +18,7 @@
"types": "./dist/src/index.d.ts",
"browser": {
"./src/lib/multipart-request.js": "./src/lib/multipart-request.browser.js",
"ipfs-utils/src/files/glob-source": false,
Copy link
Author

Choose a reason for hiding this comment

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

@yurtsiv
Copy link
Author

yurtsiv commented Apr 20, 2021

I'm not sure those checks are failing due to my changes. Are they? If so, can you provide some more info on what's wrong exactly, because I can't find anything directly related to my changes in the logs.

@yurtsiv yurtsiv changed the title Reference only local files in package.json "browser" field chore: reference only local files in package.json "browser" field Apr 20, 2021
@achingbrain
Copy link
Member

Please could you rebase on top of, or merge master into your branch? There's been quite a bit of CI instability recently that should now be resolved.

achingbrain pushed a commit to ipfs/js-ipfs-utils that referenced this pull request Apr 28, 2021
@yurtsiv yurtsiv requested a review from achingbrain April 30, 2021 17:23
@yurtsiv
Copy link
Author

yurtsiv commented Apr 30, 2021

Again not sure why it fails. I did the rebase.

@achingbrain
Copy link
Member

@yurtsiv have you tried running the failing tests locally?

@yurtsiv yurtsiv force-pushed the meteor-problem branch 2 times, most recently from 9c2b6ee to a5d1cf1 Compare May 7, 2021 18:55
@yurtsiv
Copy link
Author

yurtsiv commented May 7, 2021

@achingbrain Honestly, I don't know how to run individual tests since I'm not that familiar with your setup and running all tests globally is taking me forever. I just realized that I didn't remove references to ipfs-utils/src/files/glob-source in other packages. See the last commit. Maybe that's the cause of failing tests.

@yurtsiv
Copy link
Author

yurtsiv commented May 7, 2021

@achingbrain Still failing. Do you have an idea of what could go wrong?

Here're the failing tests:

ipfs:   1) interface-ipfs-core ipfs-client tests
ipfs:        .files.ls
ipfs:          with sharding
ipfs:            lists a sharded directory contents:
ipfs:      TypeError: content is not async iterable
ipfs:       at sendFile (file:///home/travis/build/ipfs/js-ipfs/packages/ipfs-grpc-client/src/core-api/add-all.js:52:27)
ipfs:       at sendFiles (file:///home/travis/build/ipfs/js-ipfs/packages/ipfs-grpc-client/src/core-api/add-all.js:100:13)
ipfs: 
ipfs: Command failed with exit code 1: pw-test test/interface-client.js --mode main --config /home/travis/build/ipfs/js-ipfs/node_modules/aegir/src/config/pw-test.js --timeout=5000 --bail
example-browser-script-tag: Running browser tests in /home/travis/build/ipfs/js-ipfs/examples/browser-script-tag
example-browser-script-tag: Found bare file /home/travis/build/ipfs/js-ipfs/examples/browser-script-tag/index.html
example-browser-script-tag: Running tests at file:///home/travis/build/ipfs/js-ipfs/examples/browser-script-tag/index.html
example-browser-script-tag:    Error: ChromeDriver process exited with code: 1
example-browser-script-tag:    [1620411305.981][SEVERE]: bind() failed: Cannot assign requested address (99)
example-browser-script-tag:   [1620411305.981][SEVERE]: bind() failed: Address already in use (98)
example-browser-script-tag:   
example-browser-script-tag:        at Process.ChildProcess._handle.onexit (internal/child_process.js:277:12)
example-browser-script-tag:  [1620411305.981][SEVERE]: bind() failed: Cannot assign requested address (99)
example-browser-script-tag:  [1620411305.981][SEVERE]: bind() failed: Address already in use (98)
example-browser-script-tag:  
example-browser-script-tag: npm ERR! code ELIFECYCLE
example-browser-script-tag: npm ERR! errno 10
example-browser-script-tag: npm ERR! [email protected] test: `test-ipfs-example`
example-browser-script-tag: npm ERR! Exit status 10
example-browser-script-tag: npm ERR! 
example-browser-script-tag: npm ERR! Failed at the [email protected] test script.
example-browser-script-tag: npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
example-browser-script-tag: npm ERR! A complete log of this run can be found in:

@achingbrain
Copy link
Member

I don't know how to run individual tests since I'm not that familiar with your setup

Have you read the developer docs?

In brief, from the root of the repo:

$ npm run test:interface:client -- -- -- --grep 'lists a sharded directory contents'

or from the packages/ipfs directory:

$ npm run test:interface:client -- --grep 'lists a sharded directory contents'

Do you have an idea of what could go wrong?

You've changed the browser override for normalise-input.js - this swaps out the content field of the imported files from AsyncIterable<Uint8Array> for Blobs in the browser.

Since the error message is content is not async iterable, my guess would be that something is now getting a Blob when it expects an AsyncIterable<Uint8Array>.

@yurtsiv
Copy link
Author

yurtsiv commented May 11, 2021

Have you read the developer docs?

No, thanks for the link.

You've changed the browser override for normalise-input.js - this swaps out the content field of the imported files from AsyncIterable for Blobs in the browser.

What do you think would be a better solution:

  1. Make ipfs-grpc-client work with Blob in the browser?
  2. Use ipfs-core-utils/src/files/normalise-input/index directly instead of ipfs-core-utils/src/files/normalise-input?

The second one is definitely easier, but seems kind of wrong IMO, since it breaks the module encapsulation.

@lidel lidel assigned hugomrdias and unassigned olizilla May 17, 2021
@hugomrdias
Copy link
Member

There's 2 issues here:

  • ipfs-core is not swapping normalise-input but http-client is, it should be done in ipfs-core-utils
    • unixfs does not support browser version of normalise-input and the types also do not support it
    • why do we need the browser normalise-input ?
  • globSource should be handled better
    • interface-core tests that use globSource should be moved to ipfs-core node specific tests
    • ipfs-core and http-client should either stop exporting globSource or split the entry point for browser and node.

@lidel
Copy link
Member

lidel commented May 25, 2021

@hugomrdias I did not write down AI for this one. If I remember correclty, we wanted to close this PR and create two more specific follow-up issues based on your last comment?

@yurtsiv
Copy link
Author

yurtsiv commented Jun 1, 2021

Hi. Any update on this one? I'm happy to work on any of the issues you described, as long as a bit more guidance is provided.

@lidel
Copy link
Member

lidel commented Jun 14, 2021

@yurtsiv I'm lacking the full context here but I believe the takeaway is:

  • during triage it was decided to close this PR – issue described in ipfs-http-client doesn't work in the browser with Meteor  #3557 is a bug in non-standard hybrid environment that we don't officially support, so we need to be very careful about introducing PR like this one, as it will impact more widely used runtimes.

following up on Hugo's comment (#3630 (comment)) we discussed this a bit during triage:

  • normalise-input – every time we make any change here, we need to make sure it works correctly in ipfs-http-client package in browser context (we use native browser types to avoid buffering entire file in memory during upload).

  • globSource – afaik this is useful only in nodejs context and only when full node is used, and it seems we already track type cleanup in Type helpers assigned to ipfs-http-client constructor #3450, this could be tackles in a separate PR

If you feel strongly about this feel free to submit PRs that address/conform to the above + ask any open questions there or on in #3557

@lidel lidel closed this Jun 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants