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

Conversation

haadcode
Copy link
Contributor

This PR will comform with ipfsd-ctl@0.19.0 which changed the .apiAddr to be a Multiaddr instead of a string.

We return that apiAddr from the node factory and due to SocketIO not handling objects and buffers successfully, we return a string. That string then gets converted back to Multiaddr in the client factory which attaches the Multiaddr to the ipfs api (like ipfsd-ctl does).

I also added a test for the constructor to accept Multiaddr both as a string and as a proper Multiaddr type.

@haadcode
Copy link
Contributor Author

Oh, and this PR will make js-ipfs-api work again with ipfsd-ctl (0.19.0)

@dignifiedquire
Copy link
Contributor

@haadcode I think ipfsd-ctl was released, can you update the PR so CI gets happy?

@dignifiedquire
Copy link
Contributor

I see you did that in #526. Can we put these two PRs together, or is there a reason for them being seperate?

@haadcode
Copy link
Contributor Author

Sure! Added it to this PR. Now pulling in ipfsd-ctl@0.19.0

@haadcode
Copy link
Contributor Author

CI is now erroring on the name tests which are fixed in #524 and #493. I'm gonna add the fixed tests to this PR.

Change node factory to pass Multiaddr to the api instead of string
Use ipfsd-ctl v0.19.0 which includes go-ipfs 0.4.5
Fix name tests
@haadcode
Copy link
Contributor Author

Squashed the commits and updated the commit message for changelog.

@haadcode haadcode changed the title Change node factory to pass Multiaddr to the api instead of string Use ipfsd-ctl 0.19.0 and go-ipfs 0.4.5 Feb 20, 2017
@dignifiedquire
Copy link
Contributor

Code lgtm, lets get travis happy

@haadcode
Copy link
Contributor Author

I don't quite understand what/why the Travis errors are. There seems to be Syntax Errors coming from webpack, and I see an OOM at the end. @dignifiedquire can you take a quick look and see if you know what the problem is?

Copy link
Contributor

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

CI is still not happy


script:
- npm run test:node
- npm run test:browser
Copy link
Contributor

Choose a reason for hiding this comment

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

why separate?

"test:browser": "gulp test:browser",
"test": "node --max_old_space_size=4096 node_modules/.bin/gulp test",
"test:node": "node --max_old_space_size=4096 node_modules/.bin/gulp test:node",
"test:browser": "node --max_old_space_size=4096 node_modules/.bin/gulp test:browser",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? I believe we fixed the webpack hungryness when we migrated all the tests to use factory only

host: splitted[2],
port: splitted[4],
host: host,
port: port,
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

it('host, port', (done) => {
const splitted = apiAddr.split('/')
it('multiaddr (string), opts', (done) => {
clientWorks(ipfsAPI(apiAddr.toString(), { protocol: 'http' }), done)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to specify the protocol now ?

@haadcode
Copy link
Contributor Author

This PR was originally made to update ipfsd-ctl to 0.19.0 (go-ipfs 0.4.5). We have since merged go-ipfs 0.4.7 to master with ipfsd-ctl 0.20.0. I believe this PR is not needed anymore.

@diasdavid is there anything we need from this PR that is not already in master (eg. tests)? If not, let's go ahead and close this PR.

@daviddias
Copy link
Contributor

Yeah, seems it is all good. Thank you either way :)

@daviddias daviddias closed this Mar 22, 2017
@daviddias daviddias deleted the fix/apiaddr branch March 22, 2017 14:49
@daviddias daviddias removed the ready label Mar 22, 2017
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