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

add opts param to IpfsAPI, allow 'protocol' config #67

Merged
merged 14 commits into from
Nov 22, 2015

Conversation

ckeenan
Copy link
Contributor

@ckeenan ckeenan commented Sep 26, 2015

Should resolve #66

@victorb
Copy link
Contributor

victorb commented Oct 19, 2015

Is there a reason why we don't just make all the arguments opts? Instead of having host and port on separate arguments, they would be in opts as well. Ala #71 (comment)

@daviddias
Copy link
Contributor

@victorbjelkholm I like that idea, but can we make it in a way that doesn't break current usage? (when we release a ipfsAPI 3.0.0 we can make breaking changes like that)

@ckeenan thank you for the PR, would you mind rebasing master first, so it gets a clean merge? + This seems to solve the problem also pointed out at #123 //cc @dignifiedquire ?

@ckeenan
Copy link
Contributor Author

ckeenan commented Nov 20, 2015

@diasdavid rebased

@daviddias
Copy link
Contributor

LGTM. Just missing the documentation on the README.md.

@dignifiedquire @victorbjelkholm want to give your CR?

@dignifiedquire
Copy link
Contributor

For this PR please add docs in readme, a test checking this works and move the build into a separate commit.


In general I think this is not the right approach though, what @jbenet mentioned makes much more sense we should start supporting new API('https://127.0.0.1:5001') and other forms of of addresses rather than forcing people to pass in the parsed values.

@daviddias
Copy link
Contributor

Currently, ipfsAPI supports multiaddr https://github.com/ipfs/js-ipfs-api/blob/master/src/index.js#L19-L21, however it is expecting a /ip4/<ip>/tcp/<port> to use with http. Would it feel more right if it expected /ip4/<ip>/http/<port>? I would tend to say yes, but might be confusing to everyone thinking on the tcp ip:port way.

@dignifiedquire
Copy link
Contributor

I think we should support:

'http://localhost:5001'
'//localhost:5001'
'/ip4/127.0.0.1/http/5001'
{ host: 'localhost', port: 5001, protocol: 'http'}

@ckeenan
Copy link
Contributor Author

ckeenan commented Nov 21, 2015

@diasdavid @dignifiedquire updated. Thoughts?

@@ -766,4 +766,49 @@ describe('IPFS Node.js API wrapper tests', () => {
})
})
})

describe('constructor parameters', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put this into a new file, this is getting out of hand

@dignifiedquire
Copy link
Contributor

Thanks @ckeenan functionality wise this looks good, left some cr comments

@ckeenan
Copy link
Contributor Author

ckeenan commented Nov 21, 2015

@dignifiedquire I updated with your requests. For multiple test files I parse the karma config's files for the mocha task, let me know if you want that handled differently

@@ -47,7 +47,8 @@ module.exports = function (config) {
basePath: '',
frameworks: ['mocha'],
files: [
'test/tests.js'
'test/tests.js',
'test/constructor.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to rename the files to all in end .spec.js, so we can do test/*.spec.js

Copy link
Contributor

Choose a reason for hiding this comment

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

That way you can pass the same glob to the mocha config in tasks/test.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Added

@dignifiedquire
Copy link
Contributor

Looks like you need to rebase on master again sorry :(

@ckeenan
Copy link
Contributor Author

ckeenan commented Nov 21, 2015

No worries @dignifiedquire

@dignifiedquire
Copy link
Contributor

Thanks a lot. LGTM. So let's wait for @diasdavid to check and then we can ship this :)

@daviddias
Copy link
Contributor

This is awesome, thank you @ckeenan and @dignifiedquire for reviewing it :) LGTM, going to merge and release

daviddias added a commit that referenced this pull request Nov 22, 2015
add opts param to IpfsAPI, allow 'protocol' config
@daviddias daviddias merged commit 0784c5c into ipfs-inactive:master Nov 22, 2015
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.

Configurable http.request protocol option
4 participants