-
Notifications
You must be signed in to change notification settings - Fork 62
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
feat: bring your own API client #257
Conversation
BREAKING CHANGE: library consumers must now npm install ipfs-api manually License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>
@alanshaw while I like this PR and we've gone back and forth on doing it like this a few times, it's worth pointing out that, this is already possible if using the |
@dryajov noted. I did see that being done in Workaround aside, IMHO it's odd to allow people to use their own IPFS node at whatever version but force them to use an API that might not be compatible with it. It's also worth noting that when using that workaround in the browser, If this PR isn't merged then the workaround needs to be documented here instead and I will send a PR to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this solution, and I agree with @alanshaw. We should not allow the usage of an API that might not be compatible with the node.
Nevertheless, I would like to have @diasdavid input on this.
@alanshaw I tried arguing that point in - #195 (comment) 😀, and something like this was going to be my ultimate solution, I just never really needed it. But that said, I'm 💯 in favor of it. |
Remember that the purpose of this module is not to be a test utility, but actually a module that makes it really nice to spawn and control daemons from JS. I see two things being proposed here:
The current setup is already cumbersome. Users have to install manually a go-ipfs/js-ipfs node to make this module work. The only reason why js-ipfs and go-ipfs are not bundled here is because js-ipfs caused troubled for electron apps to build (e.g IPFS Desktop). It seems that won't be a issue anymore however -- ipfs/js-ipfs#1351 (review) --. So we should reconsider bundling it all again. That said, yes, a user should be able to inject their own API client. |
Just to be clear. I'm not against of "educating the users to bring their own ipfs and API client", however, that does change the intent of the module and it doesn't solve the issue all together, as if users pick the wrong ipfs-api and ipfs daemon combination, they might get things breaking anyway and the errors might be even from ipfsd-ctl internals. I advise that we have sane/easy to use defaults and then advanced options for power users (e.g us with our testing). |
I'm +1 on that. If it is now possible to bundle |
BREAKING CHANGE: library consumers must now
npm install ipfs-api
manuallyThis PR resolves #254 and allows people to also use their own API client. It addresses the problem of the
ipfs-api
dependency inipfsd-ctl
not being up to date with the latestjs-ipfs
orgo-ipfs
and would also allow people to use an older IPFS with a compatible API client.Most importantly, it also means we won't have to spend PR's like this to get the tests to pass on
js-ipfs
.