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

Convert messages to serialized JSON format [BREAKING CHANGE] #39

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jhobz
Copy link
Contributor

@jhobz jhobz commented Mar 10, 2022

Additionally, all commands now respond with a message indicating their success or failure

jhobz added 3 commits March 10, 2022 14:27
Require incoming messages to be serialized JSON strings. This is part 1
of a 2-part change to convert to JSON messaging.
All commands now respond with a message consisting of a JSON string
Also adds 4 previously-undocumented commands to list
jhobz added a commit to jhobz/node-livesplit-client that referenced this pull request Mar 10, 2022
@jhobz
Copy link
Contributor Author

jhobz commented Mar 10, 2022

I could use some help testing all of the set commands. I don't see why they shouldn't work, but I focused mainly on testing the getters since that's what my personal project was already set up for.

@satanch
Copy link
Contributor

satanch commented Mar 16, 2022

In my opinion this breaking change must be changed. Server could work with string commands as always, but if we need to switch to JSON messaging we could do it via mode JSON command (for example).

Algorithm example

  1. We are connecting to server as always.
  2. Default messaging protocol for the current user connection socket is 'commands'. So we could send commands as usually.
  3. Client sends message to server: mode JSON
  4. Server sends {"success": true} or nothing if server is using old LiveSplit.Server version.
  5. Server switches messaging protocol to JSON only for this socket.
  6. Now server and client could communicate via JSON but only at current connection.

Why?

  1. Great backwards compatibility.
  2. Another messaging "protocols" boilerplate will be ready.

updated

@jhobz
Copy link
Contributor Author

jhobz commented Mar 16, 2022

Respectfully, that sounds like an unnecessary complication. Existing clients that don't want to deal with the breaking change could simply keep the older version. Using a standardized communication protocol like JSON as the default opens up many more options for advanced communication between the client and server, many of which the text-based protocol would not be able to implement (or at least not cleanly).

For example, a feature idea I had for the future would be to have a getallinfo [comparison] command that could return all types of info in the data object on the response. This would reduce the number of calls back and forth between client and server, meaning fewer opportunities for packages to get missed or received out of order. A text-based protocol would likely not receive this update, which means you'd then be maintaining two different "versions" of the component.

Optional parameters (like nonces) are also notoriously difficult to do with just a space-delimited string.

There's nothing wrong with a breaking change as long as the messaging is clear. If the version number is properly updated and the changelog properly warns about the change, existing clients can be prepared and either make the changes or simply not update (losing no functionality anyways).

@wooferzfg
Copy link
Member

Not sure if it's something we want, but we also have the option of changing the URL of the XML with the list of updates, which would allow us to disable automatic updates from the current version to the version with the breaking changes.

@jhobz
Copy link
Contributor Author

jhobz commented Mar 16, 2022

Not sure if it's something we want, but we also have the option of changing the URL of the XML with the list of updates, which would allow us to disable automatic updates from the current version to the version with the breaking changes.

I'd defer to your judgment for release specifics. I think either way the semver must be updated to the next major number (so 2.0, in this case) to indicate a breaking change.

@wooferzfg
Copy link
Member

Sorry for the delay on this PR. I am comfortable with merging this once this is fully tested, but I haven't had the time to do that myself. If you're confident that this all works correctly, let me know and I can merge.

@satanch
Copy link
Contributor

satanch commented Jul 3, 2022

Any updates?

@DavidCEllis
Copy link

Is this still active?

I have something that uses the current implementation of the server and just want to be prepared if/when this gets merged.

Switching to JSON requests/responses does seem like a good idea.

Looking at it there are a couple of things I'd ask for:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants