-
Notifications
You must be signed in to change notification settings - Fork 8
Give records a reference to the model manager which loaded them #754
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
Conversation
35951ec
to
d6e6704
Compare
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.
d6e6704
to
c07859e
Compare
* The manager class for a given model that uses the Public API, like `api.post` or `api.user` | ||
**/ | ||
export interface AnyPublicModelManager< | ||
FindOneFunc extends AnyFindOneFunc = AnyFindOneFunc, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kind of unfortunate but addresses an issue with useSession
and useAuth
typing. For both these react hooks we accept a type that is:
export type ClientWithSessionAndUserManagers<SessionGivenOptions, SessionSchemaT, UserGivenOptions, UserSchemaT> = AnyClient & {
currentSession: { get: GetFunction<SessionGivenOptions, any, SessionSchemaT, any> };
user: { findMany: FindManyFunction<UserGivenOptions, any, UserSchemaT, any> };
};
js-clients/packages/react/src/auth/useAuth.ts
Lines 21 to 22 in e3773e9
Client extends ClientWithSessionAndUserManagers<SessionGivenOptions, SessionSchemaT, UserGivenOptions, UserSchemaT>, | |
ClientType extends Client | undefined |
Which scopes the client down to one that has both user
findMany
function and a session
with a get
function. This is narrowing the type of the ModelManager
and the resulting object is then typed to be a GadgetRecord
based on the input client/model manager. In order to make it play nicely with hydration and keeping the reference to the model manager I want to also add the AnyPublicModelManager
or AnyPublicSingletonModelManager
interface too. I had changed it to be:
export type ClientWithSessionAndUserManagers<SessionGivenOptions, SessionSchemaT, UserGivenOptions, UserSchemaT> = AnyClient & {
currentSession: { get: GetFunction<SessionGivenOptions, any, SessionSchemaT, any> } & AnyPublicSingletonModelManager;
user: { findMany: FindManyFunction<UserGivenOptions, any, UserSchemaT, any> } & AnyPublicModelManager;
};
however this leads to an infinite typing/complexity error getting the output GadgetRecord
type. To address this I have added these optional generic typings to allow the function typing to be passed through. This way the get
and findMany
function types can be passed through and the resulting type can be inferred again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awkward but this makes sense
/** | ||
* Prior to 1.1 actions were defined to accept just a connection | ||
*/ | ||
export interface AnyLegacyModelManager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to address a test case inside operationRunner
that asserts before 1.1 actions were typed with just a { connection: GadgetConnection }
. I'm not fully aware if this still needs to be supported, its not the end of the world as we can check the typing inside the record when adding the additional methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after comments
* The manager class for a given model that uses the Public API, like `api.post` or `api.user` | ||
**/ | ||
export interface AnyPublicModelManager< | ||
FindOneFunc extends AnyFindOneFunc = AnyFindOneFunc, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awkward but this makes sense
This resurrects #273 PR
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 thecreate
function that we get by reference to theapi.post
object. The generated client will need to decorate thecreate
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 likerecord.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.... a description that explains what, why, and how ...
PR Checklist