Skip to content

Conversation

@andremedeiros
Copy link
Collaborator

No description provided.

@andremedeiros andremedeiros requested a review from a team April 24, 2019 20:24
@michaelsbradleyjr
Copy link
Contributor

Awesome! We've bee needing to do this for quite some time, thanks for taking care of it.

@@ -1,4 +1,4 @@
const IpfsApi = require('ipfs-api');
const IPFS = require('ipfs-http-client');
Copy link
Contributor

Choose a reason for hiding this comment

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

One small thing, not really a PR blocker. Usually in .js and .ts all caps is used for constants, so it might be a little confusing to have a "library variable" that looks like a constant later on in the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm using all constants because IPFS is an acronym, like HTTP and TCP. This is also, technically, a constant 😉

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd go with Michael here. I'd even rename it ipfsClient so it's clear what it is.

@andremedeiros
Copy link
Collaborator Author

DO NOT MERGE. This version of ipfs-http-client declares as minimum engine Node >= 10.0.0, but there were discussions on ipfs/js-ipfs-http-client#937 to bring it back down to Node 8. I've commented there to check whether that's still the intention with an offer to help.

Copy link
Collaborator

@jrainville jrainville left a comment

Choose a reason for hiding this comment

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

Nice job.
This will need changes in the website too probably.

@@ -1,4 +1,4 @@
const IpfsApi = require('ipfs-api');
const IPFS = require('ipfs-http-client');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd go with Michael here. I'd even rename it ipfsClient so it's clear what it is.

}

this.events.emit('runcode:register', 'IpfsApi', require('ipfs-api'), () => {
this.events.emit('runcode:register', 'IPFS', require('ipfs-http-client'), () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This breaks embarkjs-ipfs which relies on the variable IpfsAPI

@michaelsbradleyjr
Copy link
Contributor

PR was submitted to the ipfs team: ipfs-inactive/js-ipfs-http-client#996.

@michaelsbradleyjr
Copy link
Contributor

The PR was merged and a new version of package ipfs-http-client has been published to the npm registry:

ipfs-inactive/js-ipfs-http-client@84117a1

https://www.npmjs.com/package/ipfs-http-client/v/31.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants