Update to Recursive Type reference

[nlaurie]

Completion the the Pattern of TInsertType. I found a Type recursion issue with my complex class types dealing with the updating and hooks. Specifically the (UpdateSpec) since it generically looks at the passed in Type recursively it suffers from circular references on complex class types. So I completed the pattern by passing along the TInsertType respectively. if specifying the TInsertType it should be used universally to prevent leaky abstraction making it's way into Dexie Entity processing

Nick

[dfahlander]

Thanks for your PR! I understand that there is some kind of type recursion that need to be solved and I'm glad you found a way to fix this. Just thinking whether this could make it impossible to change realmId or owner on DexieCloudTable entries (or other types where TInsertType has the props optional. I suppose not though as UpdateSpec should allow changing optional props as well.

I'll be looking more into it later today or monday at latest.

[dfahlander]

Looking at it again, I believe the creating hook should take a TInsertType while updating and reading hook take a T, because the TInsertType is the type passed to Table.add() and T is the type actually stored in the database and retrieved from it.

I'm also curious to understand how these changes avoid the recursiveness. Might it be something that could rather be solved in UpdateSpec?

[nlaurie]

I believe you are correct the main recursion (Bug) happened in the "UpdateSpec" When using a Complex Class structure the TInsertType keeps a nice abstraction layer between the actual class and the stored data. The important thing to understand is the DataClass should theoretically always implement the TInsertType. So even if you pass the Class to the insert or update it is interface compatible and there is no need to analyze the Class Type for a partial update etc.. Even if it could be solved with a better UpdateSpec it would be extremely hard to account for every complex class structure other than the most basic DataObjects.

I added the TInsertType to the reading hook because when deserializing to a class object the data coming from the IndexedDb is the raw TInsertType, then in the hook receives that TInsertType and returns the Deserialized Real DataClass type T. I was basing this off the mapToClass function. I created a special version of this class to do a more sophisticated deserialization but it basically followed the same pattern as the original. I didn't apply the change to the Creating hook as I was not using it, but I believe the application would be the same with the TInsertType as that is the closest type to what is actually being stored. I wasn't 100% which is why I didn't touch it. One could make a case that all the hooks should be the TInsertType as it's such a low level and what matches what is stored in the IndexDb.

I guess when using a custom deserializer or there is a chain of readers this may be problematic since the structure should always be the same in the hook chain. The only way to solve it if I'm understanding it correctly is have a single instance hook api for deserialization. That way only "One" process (method) is responsible for going from the serialized structure to the Class instance per table, Then from there on the class instance can be passed to all the extensions hooking Dexie ie. reading would pass around the DataClass T.

kinda like a singleton hook "deserialize". the native mapToClass would register it and if someone needed their own serialization they could inject their own mapToClass implementation. or take over the "deserialize". rendering the native mapToClass disabled for a particular table.

Seems like mapToClass suffers from the same issue of multiple readers , it would have to be first in the chain also the first reader gets the TInsertType converts it and the rest of the listeners would get a different Class Type , now I'm understanding the tension with the Typing the reading hook.

One could take a position that all extensibility via hooks and dbcore is done in the TInsertType format allowing Dexie and any plugins/third parties to rely solely on the serialized JSON structure which matches ultimately what is being stored in the DB "POJO", and anything being returned via queries to the end user of the library either gets the T Type which would be the POJO or a deserialized Class if using mapToClass or a custom deserializer. This could help prevent against a leaky abstraction.

Happy to brainstorm some more if needed...

[nlaurie]

Any update to this? There still is a Recursive Typing Error in the master branch.

[nlaurie]

I understand about the read hook being the object type. I believe that change was mainly base on the way mapToClass was implemented in the core. The use of this method the mapping process would have to assume the obj: Object is the TInsertType to then return T. there is definitely is a discrepancy if there are down stream readers that want to only interact with the TInsertType.

Maybe there is a more concrete way to do class mapping that only allows the ONE transformation (class mapping) operation at the end of the reading pipeline -> to a new fetching? pipeline that then passes the T type (class) along. That way a simple JSON is the result of the reading sent to a replaceable (mapToClass) which then starts the fetching hook for consumers only interested in the final T type (which most likely will be the project owner)

// Reading is for the underlying serialized indexDb TInsertType
// Like with encryption and other layers that directly change the POJO , there is no worry of getting a complex class type that cause many issues with generic manipulation.
(eventName: 'reading', subscriber: (obj:TInsertType) => TInsertType | any): void;

// Fetching is for the mapped class type T (if the user is doing class mapping)
(eventName: 'fetching', subscriber: (obj:T) => T): void;

[nlaurie]

Also, I believe the Type Recursion is mainly happening with

Table Type

update(
key: TKey | T,
changes: UpdateSpec | ((obj: T, ctx:{value: any, primKey: IndexableType}) => void | boolean)): PromiseExtended;

The UpdateSpec should be --- UpdateSpec

Since Updating is a partial operation direct to the database. It makes more sense for a user to assume a Partial version TInsertType similar to the add() or put()

When giving the Generic UpdateSpec a complex class type full of getters , references to other classes etc. It is failing to traverse all but the most simple interface types.

[dfahlander]

My thoughts is that the type passed to reading hooks should be a Pojo<T> rather than TInsertType. Pojo<T> could strips methods.

Was the last commit on dexie export import intentional?

[nlaurie]

I reverted my changes with the reading hook , everything is fine there at least it still works being cast as any. It's the UpdateSpec in the table.d.ts type that needs to be UpdateSpec. which will fix the Recursion Issue.

I can undo my Branch and just submit the PR for this change by itself. The import / Export changes were submitted under a different branch and PR. I may have accidentally committed them to the TypesFix Branch.

[dfahlander]

I reverted my changes with the reading hook , everything is fine there at least it still works being cast as any. It's the UpdateSpec in the table.d.ts type that needs to be UpdateSpec. which will fix the Recursion Issue.

I can undo my Branch and just submit the PR for this change by itself. ...

I see. I really apprechiate the work you've done on this and I think dexie4 might have 2 issues: 1) the recursion issue (that would be fixed in the UpdateSpec), then we have the fact that reading hooks typings lie if T is a class with methods - which is perfectly doable with mapToClass etc. I think the PR as it is is pretty close to solving them both, so I could also take over and introduce a Pojo<T> generic to use in reading hook and possibly other places where we get data directly from db without having wrapped them in a class instance yet.

[dfahlander]

@nlaurie Would you mind checking #1897 would solve your case? It's based on your branch but I introduced the Pojo type instead to get more proper typings for the hooks. Basically:

• reading hook - Expects a Pojo version of the instance and to return such also. It's expected to return the same, but since the full class version is a superset, it wouldn't be wrong either.
• creating hook - expectes the TInsertType because that's what's actually being sent to it - the optional properties like an incremented primary key may be missing.

I haven't tried whether this solves the type recursion issue but maybe you have a chance to see if it solves it the same way as your original PR?

[nlaurie]

Line 48 in table.d.ts. Fixes the Recursive Typing issue.
This is fine
(eventName: 'creating', subscriber: (this: CreatingHookContext<TInsertType,TKey>, primKey:TKey, obj:TInsertType, transaction:Transaction) => void | undefined | TKey): void;

As far as the pojo.

short answer
I think you can leave it as
(eventName: 'reading', subscriber: (obj:T) => T | any): void; Existing

Long Answer
I don't believe passing around the direct type T should pose any issues for consumers of the hooks , external parties really won't know what the objects are and will have to deal with (any) for the most part. more importantly they will have to understand they may get a class || simple object and deal with that. Just as they would in DBCore. And really understand what they are doing if replacing the Object

and someone with their own db implementation who is interested in these hooks will want the fully typed T during the execution of the hooks.

It really depends on who are the consumers of the hooks. I believe ultimately it's the developer writing a application and wants to get events on the DB but not go through the hassle of writing a DBcore Plugin. In that case they care only about T and TInsertType. Their Class Object and Serialized Representation.

I would only add one thing and its for people that want to implement their own serialization and/or class mapping. The built in one was too generic for our project so we just followed the same pattern and copied the internal mapToClass.

That lead to using the reading hook in a funny way. since the reading hook passes in one thing and then replaces it with another and this can happen multiple time if there are many listeners (plus order may matter) it works decent for simple objects but complex stateful classes the user may not want anyone replacing them

Technically there should only be once place in code that goes form the DB Structure TInsertType to the final exposed T given to the consumer of dixie. (noted in many simple cases those types will be the same and not mapped) , but for the more advanced app you have the option of working with Class Objects or enhanced Objects.

maybe allow the user to replace the mapToClass function when defining the Table. ie.

1. register the default mapper
2. register custom mapper

I could take a stab at the implementation. that way there is an internal single place inside Dexie that gets called if/when mapping is necessary. the result of the function is always T. then whenever Dexie needs the T it has a non leaky abstraction to go from TInsertType to T , and users can implement there own serialization.

[dfahlander]

Leaving the reading hook as you suggested.

Tried to repro issues with recursive types and add it in test-typings but I couldn't find a way (don't know how to repro).

We still have a similar signature for Collection.modify() in collection.d.ts where it takes an UpdateSpec<T>.

[nlaurie]

I can try and create a simple environment to repro it.

Here is the main compiler error ( our code base has over half million lines of code so may be hard to pin down the exact source) ultimately by avoiding complex type manipulation on the type T you are abstracted from any complex deserialized classes

error TS2615: Type of property 'tables' circularly references itself in mapped type '{ [P in keyof Dexie]: P extends string ? Dexie[P] extends (infer K)[] ? K extends object ? P | ${P}.${number} | ${P}.${number}.${KeyPaths<K>} : P | ${P}.${number} : Dexie[P] extends (...args: any[]) => any ? never : Dexie[P] extends object ? P | ${P}.${KeyPaths<...>} : P : never; }'.

[dfahlander]

Hmmm... how does the Dexie type come in as a T there? Are you using EntityTable<T> or provide in other way provide the Dexie instance as a prop or sub prop in an entity?

[nlaurie]

Here you go, easy repro

sandbox.zip

[dfahlander]

If the issue would be due to providing the Dexie instance from a getter of an entity (like Entity<T> also does when deriving it), we could explicitely stop the recursion with an conditional type in KeyPaths:

export type KeyPaths<T> = {
  [P in keyof T]: 
    P extends string 
      ? T[P] extends Dexie ? never : // AVOID RECURSIVENESS
      ? T[P] extends Array<infer K>
        ? K extends object 
          ? P | `${P}.${number}` | `${P}.${number}.${KeyPaths<K>}` 
          : P | `${P}.${number}`
        : T[P] extends (...args: any[]) => any // Method
           ? never 
          : T[P] extends object 
            ? P | `${P}.${KeyPaths<T[P]>}` 
            : P 
      : never;
}[keyof T];

[nlaurie]

Funny thing , wasn't even aware of EntityTable. but it doesn't change anything in the repo. interesting enough when doing updates I don't even consider wanting to pass the T type to Dexie only the TInsertType , which is why UpdateSpec<TInsertType> is important

[dfahlander]

I tried the repro and it somewhat makes it clearer that you need to express different type in update/modify than what the db returns. Tried with typescript@latest also and it doesn't complain though, but doesn't either provide any decent code completion for the update method - seems it gives up on the type rather than complaining about recursion.

The TInsertType happens to be the only type to express. It's purpose was for add and put methods - mainly to get a solution for not having to have id as an optional property since we know it's there when reading objects from DB but it can be optional when adding new objects if it's auto-generated. Same goes with dexie cloud properties realmId and owner and theoretically also with other database-provided properties that are certain to be there when reading data but doesn't have to be provided when inserting new objects.

Having TInsertType in Table.update() and Collection.modify() would make it possible to work around the problems that arise from the mapped type UpdateSpec if T has recursive properties. Seems though that Typescript has "solved" this in the latest version.

A perfect solution could have been to allow a 4th argument MappedType=T to be passed to Table representing the mapped type going out from the reading hook and used when using mapToClass() - the type returned by toArray(), get() etc - a type that could have loads of getters and methods and be recursive etc without affecting Table.update() etc.

I'm not keen on introduction a 4th argument though and might go for your suggestion and use TInsertType in both Table.update() and Collection.modify() so that we can move on from here without adding too much of complexity.

[dfahlander]

Thanks for your efforts and for the repro 💯

[nlaurie]

Im starting to understand your requirements for TInsertType now, as ours are slightly different. Maybe simply renaming TInsertType to TSerialized will offer more clarity to the developers.

Our app separates the actions into three different interfaces Create, Update, PartialUpdate & Model

I would have to further understand your use with autogen PKs we handle that with the "Model" having everything and the "Create" "Update" and "PartialUpdate" omit them. its a lot of work to maintain but offers complete type safety.

I do agree to keep it simple maybe you just represent the full model and type the auto generated as optional. not perfect but allows you to put without the key and potentially rekey the object with an update. if necessary.

[dfahlander]

Merged through #1897