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

remove json patch #881

Merged
merged 4 commits into from
Jan 24, 2023
Merged

remove json patch #881

merged 4 commits into from
Jan 24, 2023

Conversation

rmorshea
Copy link
Collaborator

@rmorshea rmorshea commented Jan 20, 2023

closes: #877

Checklist

Please update this checklist as you complete each item:

  • Tests have been included for all bug fixes or added functionality.
  • The changelog.rst has been updated with any significant changes.
  • GitHub Issues which may be closed by this Pull Request have been linked.

@rmorshea rmorshea force-pushed the remove-json-patch branch 2 times, most recently from 44e1514 to f2f665e Compare January 21, 2023 00:22
@rmorshea
Copy link
Collaborator Author

rmorshea commented Jan 21, 2023

@Archmonger, at the moment, this change proposes that all messages between the server and client contain a type key. For example:

{
    "type": "layout-event",
    ...
}

This anticipates the possibility of a message routing system that components could hook into. For example, the following could be possible in the future:

@component
def Example():
    messenger = use_messenger(receives="custom-recv-type", sends="custom-send-type")

    @use_effect
    async def message_receiver():
        while True:
            inbound = await messenger.receive()
            ...
            await messenger.send(outbound)

    ...

My thinking with the single type key is that in the future, if more information is needed, any of the following could be done:

  • Version information could be communicated either by encoding it into the type string (e.g. [email protected])
  • Extra metadata could be defining new messages types with that added information.
  • Arbitrary message metadata could be communicated by defining handshake message types that the client/server open their communication with and that they use to declare what message types and versions they use. (this seems preferable)

I'm open to alternatives though. For example, adding a header key to each message which is an object containing things like the message type and version. This has the advantage of reserving a namespace under which other useful information could be added in the future. However, I think this might encourage adding a bunch of redundant boilerplate to message headers that could impact performance.

@Archmonger
Copy link
Contributor

Archmonger commented Jan 21, 2023

I support this configuration. However, I don't support encoding the version into the type string ([email protected]).

I would suggestion this format

{
    "type": "layout-event",
    "version": 1,  # `version` should be optional. If undefined, assume latest version of the API.
    ...
}

@Archmonger Archmonger linked an issue Jan 22, 2023 that may be closed by this pull request
2 tasks
@rmorshea rmorshea marked this pull request as ready for review January 24, 2023 04:19
@rmorshea rmorshea merged commit 981fd43 into main Jan 24, 2023
@rmorshea rmorshea deleted the remove-json-patch branch January 24, 2023 04:32
@Archmonger Archmonger mentioned this pull request Aug 25, 2023
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

Successfully merging this pull request may close these issues.

Remove JSON Patch Cannot update the root element.
2 participants