Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue/23 js ipfs api #53

Merged
merged 8 commits into from
Mar 12, 2021
Merged

Issue/23 js ipfs api #53

merged 8 commits into from
Mar 12, 2021

Conversation

c0rv0s
Copy link
Contributor

@c0rv0s c0rv0s commented Feb 26, 2021

The previous dependency ipfs-api has an unpacked size of 12.1 mb. This branch changes the ipfs dependency to ipfs-http-client which is only 617 kb unpacked. It's also the currently maintained IPFS client.

Additionally this branch adds a a test suite with Jest.

@c0rv0s c0rv0s self-assigned this Feb 26, 2021
@CLAassistant
Copy link

CLAassistant commented Feb 26, 2021

CLA assistant check
All committers have signed the CLA.

@c0rv0s
Copy link
Contributor Author

c0rv0s commented Mar 1, 2021

@nivida I'd like to get a code review on this. The test coverage was mostly so that I could be confident the new package is integrated correctly. A more complete test coverage is addressed here

jest.config.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/test-helpers.js Outdated Show resolved Hide resolved
test/test.js Outdated Show resolved Hide resolved
test/test.js Outdated Show resolved Hide resolved
@c0rv0s c0rv0s requested a review from nivida March 4, 2021 21:25
Copy link

@nivida nivida left a comment

Choose a reason for hiding this comment

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

Thanks for the changes :)

I have on the second review noticed something I think will otherwise break the backward compatibility.

src/providers/ipfs.js Outdated Show resolved Hide resolved
@c0rv0s
Copy link
Contributor Author

c0rv0s commented Mar 12, 2021

I added a few more changes after running the cli tests with this repo as a sym-linked package. Tested with both node 10 and 12.

@c0rv0s c0rv0s requested a review from nivida March 12, 2021 00:22
@nivida nivida merged commit 8796e5d into master Mar 12, 2021
@nivida
Copy link

nivida commented Mar 12, 2021

@c0rv0s thanks :)

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.

3 participants