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.

Sign up for GitHub

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

Jump to bottom

fix: support POST-only HTTP API #1430

Merged
merged 2 commits into from Apr 4, 2020
Merged

fix: support POST-only HTTP API #1430

merged 2 commits into from Apr 4, 2020

Conversation

lidel
Copy link
Member

@lidel lidel commented Apr 4, 2020 •
edited

Problem

ipfs/kubo#7097 will block GET commands on API port, requiring everything to be a POST request.

This breaks Files screen in ipfs-webui as noted in #1429

Even a bigger problem

ipfs-webui is using older version of js-ipfs-http-client, one before huge refactor into async iterables, which means switching to the latest version won't be a trivial task, as the amount of code and dependencies that needs to be refactored is unknown.

Solution

Due to time constraint I made a decision to patch it in npm postinstall so we can release this ASAP and ship with go-ipfs 0.5.

postinstall script will apply a simple patch on top of ipfs-http-client v39.0.2 to ensure it sends commands as POST

Proper fix will land when ipfs-webui is refactored to work with ipfs-http-client >41.x

Closes #1429

cc @achingbrain for visibility

@lidel
fix: support POST-only HTTP API
d7f330c 
ipfs/kubo#7097 will block `GET` commands on
API port, switching everything to POST.

This breaks Files screen in ipfs-webui as noted in
#1429

ipfs-webui is using older version of js-ipfs-http-client, one before
huge refactor into async iterables, which means switching to the latest
version won't be a trivial task.

For now, we just apply simple patch on top of ipfs-http-client v39.0.2
to ensure it sends commands as POST.

Proper fix will land when ipfs-webui is refactored to work with
ipfs-http-client >41.x

Closes #1429
@lidel lidel mentioned this pull request Apr 4, 2020
Closed
@lidel lidel requested review from hsanjuan and Stebalien April 4, 2020 19:55
Stebalien
Stebalien approved these changes Apr 4, 2020
View reviewed changes
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

What about shipping a patch release of the js ipfs API?

@lidel
Copy link
Member Author

lidel commented Apr 4, 2020

@Stebalien @hsanjuan
I tested this against ipfs/kubo#7097 and Files screen is fixed.
If it is okay with you I'd like to merge this and make a new release of ipfs-webui, so we can update go-ipfs to use it and also fix E2E tests in ipfs/kubo#7097

@lidel
Copy link
Member Author

lidel commented Apr 4, 2020

@Stebalien I am not sure if patch release will be possible due to recent move to monorepo.

I'd rather not bother @achingbrain with it. Keeping this fix local to webui decreases noise: everyone should be already on the latest js-ipfs-http-client >41.x anyway, its just ipfs-webui lagging behind and we have no bandwidth to upgrade it before go-ipfs 0.5.

@lidel lidel marked this pull request as ready for review April 4, 2020 20:11
@hsanjuan
Copy link
Contributor

hsanjuan commented Apr 4, 2020

Sounds good, I trust your judgment.

@lidel lidel mentioned this pull request Apr 4, 2020
Closed
@lidel
Copy link
Member Author

lidel commented Apr 4, 2020

I created #1431 to track proper upgrade.

I'll merge this and prepare a new release.

@lidel lidel merged commit 945315e into master Apr 4, 2020
@lidel lidel deleted the fix/api-post branch April 4, 2020 20:59
@Stebalien
Copy link
Member

@lidel thinking about this a bit, we should really release a patch release of the HTTP API. Upgrading to async/await is non-trivial so there are likely dapps other than ipfs-webui in the same boat.

@lidel
Copy link
Member Author

lidel commented Apr 5, 2020 •
edited

@Stebalien I agree, I hit into more problems while working on Companion and would really appreciate if such patch release existed.

Created ipfs/js-ipfs#2971 and marked it as P0 to make sure POST-only API is properly handled on JS side, including patch release.

lidel added a commit that referenced this pull request Apr 7, 2020
@lidel
fix: support POST-only HTTP API (clean way)
97e20e5 
Removes patching introduced in #1430 and uses ipfs-redux-bundle v7.0.0
to bring a compatible version of ipfs-http-client that is known to
support POST-only HTTP API while also exposing the old JS API.

Context: ipfs/js-ipfs#2971
@lidel lidel mentioned this pull request Apr 7, 2020
Merged
lidel added a commit that referenced this pull request Apr 14, 2020
@lidel
fix: support POST-only HTTP API (clean way) (#1432)
de684d7 
Removes patching introduced in #1430 and uses ipfs-redux-bundle v7.0.0
to bring a compatible version of ipfs-http-client that is known to
support POST-only HTTP API while also exposing the old JS API.

Context: ipfs/js-ipfs#2971
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stebalien Stebalien Stebalien approved these changes

@hsanjuan hsanjuan Awaiting requested review from hsanjuan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support POST-only HTTP API
3 participants
@lidel @hsanjuan @Stebalien