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

connections state needs improvement #38

Open
karth295 opened this issue Apr 7, 2018 · 6 comments
Open

connections state needs improvement #38

karth295 opened this issue Apr 7, 2018 · 6 comments

Comments

@karth295
Copy link
Collaborator

karth295 commented Apr 7, 2018

The connections state is nice to have, but it's a little confusing to use.

  1. It looks different on the client and server. On the client, fetch('/connections') -> {key: '/connections', all: [{...}, {...}]}, and on the server, fetch('connections') -> {key: 'connections', connId1: {...}, connId2: {...}}

  2. It doesn't work with the state interface -- must call fetch/save

@toomim
Copy link
Member

toomim commented Apr 7, 2018

For 1, the different isn't client vs. server, but rather the client vs. master bus on the server. For instance:

master = require('statebus').serve({
  client: (client) => {
    ...
    client.fetch('connections')   // Here you get {key: 'connections', all: [...]}
}

The client vs. master bus distinction has always bugged me, and I'm sorry that it's biting you here too. I'd like a better solution to giving each client their own state space, without confusion like this. I've also considered making a sub-space within a single bus, like perhaps /client/23423/* would substitute for the client bus.

For 2, maybe this is the same problem as 1? I just tested the state interface on the client and it works. The global state object is probably pointing at the master bus, but maybe that shouldn't exist, like you pointed out in #40.

@karth295
Copy link
Collaborator Author

karth295 commented Apr 7, 2018

Yeah that's very confusing.

Also I don't necessarily want to start a server if I just want to write unit tests. I'll open a PR to tawk to show you what I'm working on.

@karth295
Copy link
Collaborator Author

karth295 commented Apr 7, 2018

Also I think state.connections / state['/connections'] should return either an array of connections or an object with connid -> connections, rather than this weird {key: ..., all: [...]}

@karth295
Copy link
Collaborator Author

karth295 commented Apr 7, 2018

That would make unit tests in my PR a lot shorter / more readable.

@toomim
Copy link
Member

toomim commented Apr 8, 2018

I agree about the all field in connections being annoying.

The reason connections looks like {key: ..., all: [...]} is because the current version of statebus requires every piece of state to be a javascript object with a key on it. Once we get the state proxy working better, we can transition all state to the JSON encoding and make it a regular array or object.

However, I have improvements in mind for both the JSON encoding and the network protocol, which will break compatibility, and I want to implement them all at once in version 7. The new JSON encoding will simplify the implementation of the state Proxy, which makes me want to wait for it before I fix the bugs in the current implementation of the Proxy. Unfortunately, I've been entirely obsessed with the synchronization algorithm lately, and haven't put any work into the new encoding, protocol, or the Proxy.

@toomim
Copy link
Member

toomim commented Apr 8, 2018

I've listed the upcoming breaking changes here: https://wiki.invisible.college/statebus/dev

You'll see that refactoring /connections is on the list!

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

No branches or pull requests

2 participants