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

Give records a reference to the model manager which loaded them #219

Closed
wants to merge 1 commit into from

Conversation

airhorns
Copy link
Contributor

@airhorns airhorns commented Jun 28, 2023

We have a few upcoming things that require records to be able to do stuff to themselves -- reload, select new fields from the backend, maybe record.save or similar, and I suspect more in the future. I still think they should act mostly as DTOs that don't have a lot of business logic on them, but if we want to be able to even know what model they are for, we need a reference to the thing that produced them! This passes along the model manager which instantiated a record to the record in both imperative API land and in React land.

In order to make this change work in React land, we need a new little bit of metadata exported from the generated client. Our React hooks get passed functions from the api client like useAction(api.post.create), so we need to be able to hop back from the create function that we get by reference to the api.post object. The generated client will need to decorate the create function with a reference, which is not super hard, but means that we have a backwards compatibility issue. @gadgetinc/react can't assume that it is upgraded at the same time as the api client, so it can't assume it is working against a newly generated api client that has this metadata.

For this reason, the model manager property on GadgetRecord is optional, which reflects the way it will be used in the real world without necessarily having that metadata available. When we go to build things like record.reload(), we can make that fail at runtime with a message saying "regenerate your client to get this to work!", instead of just assuming it is present.

Gadget side PR that tries this out: https://github.com/gadget-inc/gadget/pull/7201

We have a few upcoming things that require records to be able to do stuff to themselves -- reload, select new fields from the backend, maybe `record.save` or similar, and I suspect more in the future. I still think they should act mostly as DTOs that don't have a lot of business logic on them, but if we want to be able to even know what model they are for, we need a reference to the thing that produced them! This passes along the model manager which instantiated a record to the record in both imperative API land and in React land.

In order to make this change work in React land, we need a new little bit of metadata exported from the generated client. Our React hooks get passed functions from the api client like `useAction(api.post.create)`, so we need to be able to hop back from the `create` function that we get by reference to the `api.post` object. The generated client will need to decorate the `create` function with a reference, which is not super hard, but means that we have a backwards compatibility issue. `@gadgetinc/react` can't assume that it is upgraded at the same time as the api client, so it can't assume it is working against a newly generated api client that has this metadata.

For this reason, the model manager property on `GadgetRecord` is optional, which reflects the way it will be used in the real world without necessarily having that metadata available. When we go to build things like `record.reload()`, we can make that fail at runtime with a message saying "regenerate your client to get this to work!", instead of just assuming it is present.
@airhorns
Copy link
Contributor Author

airhorns commented Aug 3, 2023

/cc @jasong689 I havent finished this but this was my thinking for the first step of how to build the someGadgetRecord.load("unloadedField") thing

@airhorns airhorns closed this Sep 18, 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.

1 participant