Add gateway address to the returned IPFS api #145
Conversation
|
CI is very unhappy here |
|
Fixed! |
src/node.js
Outdated
| @@ -198,6 +198,7 @@ class Node { | |||
| }, | |||
| data: (data) => { | |||
| const match = String(data).trim().match(/API server listening on (.*)/) | |||
| const gwmatch = String(data).trim().match(/Gateway (.*) listening on (.*)/) | |||
There was a problem hiding this comment.
Lets not duplicate work. The trim and string cast are happening twice now.
src/node.js
Outdated
| @@ -206,6 +207,13 @@ class Node { | |||
| this.api.apiHost = addr.address | |||
| this.api.apiPort = addr.port | |||
|
|
|||
| if (gwmatch) { | |||
| this.gatewayAddr = gwmatch[2] | |||
| const addr = multiaddr(this.gatewayAddr).nodeAddress() | |||
There was a problem hiding this comment.
How about only exposing this.gatewayAddr = multiaddr(gwmatch[2]). That way we have the standard multiaddr we are using everywhere and you can pull the details you need at any point.
There was a problem hiding this comment.
I believe following the current convention as per api host and port is a better approach for consistency.
We can do what you propose in separate PR, bearing in mind it would be a public API change.
There was a problem hiding this comment.
how is the a breaking change, currently the gatewayAddr doesn't exist from what I can tell?
There was a problem hiding this comment.
Jumped some steps here :) Let me try again:
We currently have (with this PR):
ipfs.apiHost
ipfs.apiPort
ipfs.gatewayHost
ipfs.gatewayPortIf we would expose the addresses as multiaddr, we should make sure the properties are consistent:
ipfs.apiAddr
ipfs.gatewayAddrSo with that, I would get rid of ipfs.apiHost and ipfs.apiPort, and that would be the breaking change.
Does that make sense? We could also add both ipfs.gatewayAddr and ipfs.apiAddr in this PR, but then we'd have the apiHost and apiPort still there and I think that'd be slightly more confusion to have same thing as multiple properties.
Let me know what you think!
There was a problem hiding this comment.
How about exposing gatewayAddr and apiAddr as the main thing, and making the others simple helper functions which just depend on those?
There was a problem hiding this comment.
Changed the code now to expose apiAddr and gatewayAddr in the Node and to keep api compatibility, keep the separate variables for host and port in the ipfs instance. I'm not a huge fan of the monkey-patching solution here as it changed the public API of js-ipfs-api, but that's how it's always been, so I don't want to break that now.
@dignifiedquire take a look at let me know if you think this is good to go.
daviddias
added
status/ready
haadcode
force-pushed
the
feat/gateway-address
branch
from
39d54f5 to
f308af2
Compare
|
As noticed on IRC, this will change the public property |
|
version bump will happen on release. |
Are there PRs to cope with the breaking change proposed here? |
This will change the public API of |
|
Just to be sure, can you check if js-ipfs-api runs smoothly with these changes? |
Sure thing! I'll add tests to js-ipfs-api for this today. |
This PR will add a properties to get the address the gateway was started at and can be accessed with: