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

Non-observable model properties (like observable.ref) #504

Open
graphographer opened this issue Mar 11, 2023 · 7 comments
Open

Non-observable model properties (like observable.ref) #504

graphographer opened this issue Mar 11, 2023 · 7 comments
Labels
❔ question General question

Comments

@graphographer
Copy link

In the continuing tradition of probably not using Keystone as intended, I've been creating a generic Messaging system which expresses notifications, requests, and responses as Keystone models which are synchronized via patches across a websocket.

I ran into an interesting issue, which I have solved, but I'm hoping that maybe there is an easier way.

For example, a Request has a model property called data which is intended to hold the contents of the request. Assume that the request is to apply an action call on the server, and so data should a SerializedActionCall. However, when attached to the model, the action call is made observable and argument values are sometimes transformed in odd ways that I don't understand.

The solution I've come up with is to JSON.stringify serialized action calls and then to de-stringify them in order to apply them. However, I'd like to avoid doing this if possible, so I'm wondering if there's some feature of Keystone models that I've missed that allows a property to be expressed as a plain object (akin to Mobx's observable.ref decorator).

Another theory about what was fouling me up is that my system of using patches is converting serialized action call arguments back into model instances. So perhaps instead of expressing these Messaging model changes as patches I could express them also as actions (a bit meta -- imagine model actions which apply serialized model actions).

@xaviergonz
Copy link
Owner

xaviergonz commented Mar 11, 2023

If you want to add SerializedActionCalls to the model I'd add them as an array of https://mobx-keystone.js.org/frozen-data where each frozen data is a SerializedActionCall object. Or even just use a plain mobx @observable.ref in your class, not everything needs to be driven by mobx-keystone (in this last case it won't be serialized though).

Btw, have you considered skipping action replication completely and just applying mutations over the data? (this is, sending JSON patches and applying JSON patches). That is also a valid option as long as you don't add any use effects to the actions that generated these json patches (as it should be done really)

@xaviergonz xaviergonz added the ❔ question General question label Mar 11, 2023
@graphographer
Copy link
Author

I had a feeling that frozen data might be suggested. I just didn't know how exactly to apply it to this particular situation.

I do actually use patches elsewhere in this way, and think it's probably a better method overall for our use-case (a realtime game-like application).

However, I'm still in the process of discovering patterns for balancing code readability and maintainability, hence all these questions and experiments. As we're in the process of refactoring our code to use Keystone for modelling and, somewhat separately, inversion-of-control patterns (somewhat, because I love your Contexts, but it's as yet unclear if they're appropriate to use in place of InversifyJS in all our cases).

One concern I have is with reducing the number of individual websocket message types: it's so easy to just invent new ones for every situation (e.gs. update this or that property on this or that model; request this or that model, etc, etc.). I was attracted to using Keystone to express all of our client/server interactions as actions or patches such that only one or two websocket message types would be necessary per root.

I am beginning to feel that the Messaging abstraction over our websocket messages is over-engineered and adds an unnecessary layer of complexity since it feels like it's cutting against the grain of Keystone's API.

I think it's looking like I should take my own / your suggestion(s) and settle on:

  • One websocket request type (we use a websocket library that provides http request/response-like patterns) for retrieving a root DTO
  • One websocket type for communicating/distributing patches to/from server and clients
  • More elaborate situations where auth or server-privileged data or transactions that might fail is a factor might sometimes necessitate a third websocket type for actions

Does this segregation make sense to you?

@xaviergonz
Copy link
Owner

I think with the following general messages you should be good to go:

  • server to client: first connection message where the current state of the document is sent
  • client to server: patches to send to the other clients (probably with the patches undone locally until sent back (accepted) by the server)
  • server to client: patches to apply in this particular client

Plus whatever other non-generic messages you may need.

As a security measure, you might need to separate into different "root stores" what other clients may modify (shared document) vs what they shouldn't (e.g. your username) to ensure the patches only target that shared state. Or at the very least validate the paths in the server if you want to use a common root store.

@graphographer
Copy link
Author

That was a question I was going to ask, but I think you've answered it: does it seem practical to validate patches on a shared model?

So for example, a client has a local copy of a model shared and updated by other clients via the server, and each client should only be able to update/patch their own part of that shared data.

On the client and server:

class GameState extends Model({
  positions: prop<Record<PeerId, Position>>(() => ({}))
}) {
  @modelAction
  setPosition(peerId: PeerId, position: Position) {
    this.positions[peerId] = position;
}

And then the server checks to make sure that the patch value for peerId matches the id assigned to the websocket on which the patch message was received.

@graphographer
Copy link
Author

My thinking for preferring action replication was that such validations would be more straightforward than parsing through patches.

@graphographer
Copy link
Author

When you say "with patches undone" do you mean apply the inverse patches? Do I need to guard against feedback when doing this?

@xaviergonz
Copy link
Owner

xaviergonz commented Mar 13, 2023

yes if a pessimistic update is OK. if you want an optimistic update you will need some kind of conflict resolution (if there can be any)

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

No branches or pull requests

2 participants