Feat/websocket #6
Conversation
This makes Docker deployment easier
|
|
|
@thgh just noticing that this PR awaits your signing of the CLA to move forward. |
|
I will look into it if @robbie-cahill reacts to this PR. |
| @@ -1,4 +1,3 @@ | |||
|
|
|||
There was a problem hiding this comment.
Not sure if this change is intended?
There was a problem hiding this comment.
Those are related to prettier formatting.
Here is the websocket commit: 580c8be
There was a problem hiding this comment.
i'll try and get this commit running locally, if it works we might be able to cherry pick it
|
|
||
| let config = toml.parse(fs.readFileSync('config-instance.toml').toString()); | ||
| // Read config from environment variables |
There was a problem hiding this comment.
Reading config from environment variable is something i'd like to do but it would require some extra work for myself in the production environment. At this stage i'm deploying to a DigitalOcean droplet (Linux VM) without docker, so this may be easier using dotenv .env files.
| // Override config with config.toml | ||
| // TODO: consider env vars over config.toml | ||
| try { |
There was a problem hiding this comment.
I think in this case .env files (using something like dotenv ) provide the best of both worlds of config files and env var based config.
Its a file that can be used/changed locally without a container, but then in a container they can just be ignored and the env vars injected by the container. This would be a single solution without overrides.
| clientId: string; | ||
| websocket: HostipWebSocket; | ||
| sockets?: Map<string, HostipWebSocket>; |
There was a problem hiding this comment.
Interesting, so the websocket connections get stored in this new map?
There was a problem hiding this comment.
Yep, so there are X remote websocket connections to the server, a single connection between client/server, and X local websocket connections. The string in this map identifies each remote websocket, this information is included in each message so it gets delivered to the correct local websocket.
One thing that doesn't work reliably is closed sockets: some remote errors cannot be replayed locally, error 1006 I think.
|
@thgh the PR looks good, I like the websocket implementation. But I think the files where only formatting is changed are probably are not needed. I like the idea of using environment variables over Also the CLA would need to be signed before I can merge anything (edit: looks like you've already done that, so all good on that front). |
|
This PR is quite massive, it's probably best to review per commit and cherrypick what looks best to you. Here is a handy button: I started with a commit that only does prettier formatting to reduce clutter in the later commits. (as I didn't want to turn off automatic formatting) |
|
It would be great to get this merged. @thgh's fork is solid! Thanks guys!! |
|
is this feature gonna be merged? |
Basic websocket support
Relevant diff: 580c8be
Resolves #5