Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Decouple HTTP Servers from cli/commands/daemon #1950

Merged
merged 4 commits into from
Apr 12, 2019
Merged

Decouple HTTP Servers from cli/commands/daemon #1950

merged 4 commits into from
Apr 12, 2019

Conversation

lidel
Copy link
Member

@lidel lidel commented Mar 19, 2019 •
edited
Loading

Part of an effort to run embedded js-ipfs in Brave 🦁 ipfs/ipfs-companion#664, part of #1820 too

Summary

This PR decouples HTTP servers from code jsipfs daemon.
Apart from cleanup, we need this to enable HTTP server in browser context with chrome.sockets.

There are no functional changes. I just moved non-HTTP code to src/cli/standalone-daemon.js

Background

In the past API was exposed via HTTP Server only when jsipfs daemon was run from the commandline, so src/http/index.js was also responsible for orchestration that is not related to HTTP itself. The coupling was quite bad: HttpApi basically decided on libp2p configuration: it enabled WebRTC if available, TCP transport, MulticastDNS etc.

Changes

This refactor moves code that is not related to HTTP Servers into standalone-daemon.js, which is easier to reason about, and unlocks use of HttpApi in contexts other than CLI jsipfs daemon, such as Firefox with libdweb or Chromium-based web browser with chrome.sockets APIs

With this PR, HttpApi is not only decoupled from Standalone Daemon, but can also be manually started/stopped in browser context.

I believe only ipfs-companion will be using this for now, but it is a fairly generic code.
Someone else should be able to use this in Chrome App environment:

Start

const Ipfs = require('ipfs')
const HttpApi = require('ipfs/src/http')

let node = new Ipfs(ipfsOpts)
let nodeHttpApi

node.on('start', async () => {
  // HttpApi is off in browser context and needs to be started separately
  const httpServers = new HttpApi(node, ipfsOpts)
  nodeHttpApi = await httpServers.start()
})

Stop

if (nodeHttpApi) {
  await nodeHttpApi.stop()
  nodeHttpApi = null
}
if (node) {
  await node.stop()
  node = null
}

Checklist

  • do the thing ;-)
  • fix printing of API and Gateway port in browser console
  • ensure issue with returning payload in browser context is not due to js-ipfs
    - DONE: does not block this PR (it was due to Node-specific check in Hapi, will be addressed upstream)

Refs.
ipfs/ipfs-companion#664, #1855, #1820

@ghost ghost assigned lidel Mar 19, 2019
@ghost ghost added the status/in-progress In progress label Mar 19, 2019
@lidel lidel mentioned this pull request Mar 19, 2019
Closed
4 tasks
@lidel lidel changed the title [WIP] Refactor: decouple HTTP Server from cli/commands/daemon Decouple HTTP Servers from cli/commands/daemon Apr 11, 2019
@lidel lidel marked this pull request as ready for review April 11, 2019 13:19
@lidel
refactor: decouple HttpApi from cli/commands/daemon
a3e0bd4 
In the past API was exposed via HTTP Server only when jsipfs daemon was run
from the commandline, so src/http/index.js was also responsible for
orchestration that is not related to HTTP itself.

This refactor moves code that is not related to HTTP Servers into
standalone-daemon.js, which is easier to reason about, and unlocks use
of HttpApi in contexts other than commandline jsipfs daemon,
such as Firefox with libdweb or Chromium-based web browser with chrome.sockets APIs.

Refs.
ipfs/ipfs-companion#664

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel
fix: print HTTP listeners only when run as daemon
0b9076d 
This changes behavior in web browser. Instead of printing to
console.log, it uses proper debug-based logger.

Old behavior in terminal (when run via `jsipfs daemon`) does not change.

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel force-pushed the feat/http-gateway-without-cli branch from 29a91ce to 0b9076d Compare April 11, 2019 13:23
@lidel
Copy link
Member Author

lidel commented Apr 11, 2019

Ok, should be ready, let's review it.

@lidel
test: use StandaloneDaemon in test/http-api,gateway
7c32da8 
This replaces durect use of HttpApi with StandaloneDaemon, restoring all
existing tests to operational state.

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel force-pushed the feat/http-gateway-without-cli branch from e326540 to 7c32da8 Compare April 11, 2019 19:01
alanshaw
alanshaw suggested changes Apr 12, 2019
View reviewed changes
src/http/index.js Outdated Show resolved Hide resolved
src/cli/commands/daemon.js Outdated Show resolved Hide resolved
src/cli/standalone-daemon.js Outdated
reject(err)
})
ipfs.once('start', resolve)
})
Copy link
Member

Choose a reason for hiding this comment

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

Argh, any chance you can help @pkafei writing a test for #1878 so we don't need to do this?

(completely optional and I won't block merging this!)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we pair program and work on the test together?

Copy link
Member Author

Choose a reason for hiding this comment

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

@alanshaw I'd prefer to merge this PR as-is and improve it later.

@pkafei Sure! I am booked today, but feel free to grab time slot next week: https://calendly.com/pl-lidel/ :)

alanshaw
alanshaw suggested changes Apr 12, 2019
View reviewed changes
src/cli/daemon.js Outdated
})
this._httpApi._apiServers.forEach(apiServer => {
ipfs._print('Web UI available at %s', toUri(apiServer.info.ma) + '/webui')
})
Copy link
Member

Choose a reason for hiding this comment

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

I meant move to src/cli/commands/daemon.js - where the rest of the logging already is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack, done in 1e82102

@lidel
refactor: rename StandaloneDaemon to Daemon
1e82102 
License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel force-pushed the feat/http-gateway-without-cli branch from a2a307d to 1e82102 Compare April 12, 2019 10:32
alanshaw
alanshaw approved these changes Apr 12, 2019
View reviewed changes
src/cli/commands/daemon.js
})
daemon._httpApi._apiServers.forEach(apiServer => {
print(`Web UI available at ${toUri(apiServer.info.ma)}/webui`)
})
Copy link
Member

Choose a reason for hiding this comment

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

👌

lidel added a commit to ipfs/ipfs-companion that referenced this pull request Apr 12, 2019
@lidel
fix(brave): robust ipfs.cat + content-type sniff
a0978b8 
This applies cherry-picked patches from:
ipfs/js-ipfs#1989
ipfs/js-ipfs#1950
and solves stream issues on page refresh.

Content-type sniffing is now done over a meaningful amount of bytes
instead of arbitrary number.
@alanshaw alanshaw merged commit 5f62e99 into ipfs:master Apr 12, 2019
@ghost ghost removed the status/in-progress In progress label Apr 12, 2019
@lidel lidel deleted the feat/http-gateway-without-cli branch April 12, 2019 12:03
@alanshaw
Copy link
Member

Thanks @lidel ❤️

This was referenced Apr 16, 2019
Merged
Closed
@rtpro rtpro mentioned this pull request May 14, 2022
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pkafei pkafei pkafei left review comments

@alanshaw alanshaw alanshaw approved these changes

@hugomrdias hugomrdias hugomrdias approved these changes

Assignees

@lidel lidel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lidel @alanshaw @hugomrdias @pkafei