sensedeep
DEV: infer getModel type with typesafe input
I copied the work from PR #308 and applied to latest code to get tests to pass.
I do not recommend merging this yet because it is not backwards compatible. It creates Typescript errors where the model is passed as generic because the infer type is different.
How should we proceed? Should I try and get previous generic working as well?
I was able to allow the previous generic type as well. So everything should work as expected now.
Ok I see here we have test:v2 and test:v3 which I didn't run cause CONTRIBUTING.md only indicated test. I will have a look at these. I was also using docker. Not sure if that changes the results .
Seems my tests we returning cached results so was not seeing all the type errors. The type errors should pass now but I did see some timeout errors which I am not sure if its just my machine
a problem I see in the tests are that the model and schema gets modified further down the chain which then doesn't work with Typescript cause it compares with the schema that is sent in the table constructor. Not sure if this is test specific behaviour or something that needs to work that way. I am not sure how Typescript can keep track of updated schema
Can you point me to a case of this so I can better understand.
We could change the tests to not modify the schemas. That would probably be a good direction.
I've posted some review comments on the changed files.
Given this is breaking compatibility (table constructor), we'll need to defer to our 3.x branch which we plan to explore converting the source to TS. What are your thoughts?
The changes I introduced based on the previous open PR is just adding types so I think we should be good on the backwards compatibility part. I think it would be a really good gain if we can be sure it works.
The areas where is doesn't work is if the schema is not sent via the table constructor. Would it be an actual use case where the table instance is created without the schema? If so then yes we have a problem. This could maybe be solved with typescript but it might take a lot of work as I am also just learning advanced library typescript.
It might also be possible to switch over to typescript in the repo with exactly the same functionality thus keeping us in a v2. It will be a nicer transition to v3 and there will be gains with type safe implementations in the repo.
I will go through the other questions and reply to those. On the ones where I changed tests that seem not to be related was because I ran into type issues but will confirm.
The use case of not providing the schema is pretty rare. It was in the API from the start, but the capabilities of the schema have grown and I don't know of anyone using modified schemas. It certainly breaks tools like SenseDeep.
So I don't mind it being ugly, and losing some Typing in the case where the schema is not provided to the Table constructor.
Please see the changes we're discussing in #372 regarding create() typing.
Thank you so much for your efforts on this.
To push forward, can you resolve the conflicts with Table.d.ts so I can merge.
I'll pull the PR and test it out locally and try to resolve the backwards compatibility issue and then see if we can get this merged.
No problem glad to help as I am starting a few projects using this repo. I fixed the conflict.
The src/Table.d.ts has a lot of formatting changes. I'd like to revert those so we can focus on only the changes. We'll need to stick to the prior formatting before we merge.
Anyway, I'll test it out this weekend and give it a whirl. Looking forward to this.
The src/Table.d.ts has a lot of formatting changes. I'd like to revert those so we can focus on only the changes. We'll need to stick to the prior formatting before we merge.
Anyway, I'll test it out this weekend and give it a whirl. Looking forward to this.
formatting changes like tab space etc? I might have accidentally saved with default prettier. Could we add something https://editorconfig.org/ to set these for editors?
fixed the formatting. I did discover that the model suggestion is not working. So you could add any model and its not type checking that the model exists. It was probably because of the backwards compatibility I added. I work on this again
fixed inference again and you will see in the crud test we correctly getting a typescript error when model not known.
Thank you again for your work on this. I understand this can be a bit tedious, but we're getting close.
I've checked out the PR vs the current code:
Observations:
- Builds and tests nicely -- thank you!
- Lints cleanly
- Formatting is good and I could easily compare with the current code (Thank you again).
One issue:
- The current version will detect missing required parameters in the create() API. The PR does not.
I manually applied the change for the create() typing from the current branch which does the required checking. When added to the PR, the missing required parameters are still not checked.
type CreateProperties<T> = Omit<T, Generated<ExtractModel<T>>>
export class Model<T> {
create(properties: CreateProperties<T>, params?: OneParams): Promise<T>;
In the test below, I comment out the required attribute: "name".
The screen shot for the current code is:
The screen shot for the PR is:
You can see the type for create() in the current code is CreateProperties<UserType> whereas the PR is very different.
The problem may be the getModel definition
67c68
< getModel<T>(name: string): Model<T>;
---
> getModel<T>(name: ModelNames<Schema>): Model<Entity<Schema["models"][T]>>;
The test schema is:
const Schema = {
format: 'onetable:1.1.0',
version: '0.0.1',
indexes: {
primary: { hash: 'pk', sort: 'sk', project: 'all' },
gs1: { hash: 'gs1pk', sort: 'gs1sk', project: 'all' },
},
models: {
User: {
pk: { type: 'string', value: '${_type}#' },
sk: { type: 'string', value: '${_type}#${id}' },
gs1pk: { type: 'string', value: '${_type}#' },
gs1sk: { type: 'string', value: '${_type}#${id}' },
id: { type: 'string', generate: 'ulid' },
name: { type: 'string', required: true },
email: { type: 'string', },
version: { type: 'number', required: true },
address: { type: Object, schema: {
street: { type: 'string', required: true },
city: { type: 'string' },
state: { type: 'string' },
zip: { type: 'number' },
}},
}
} as const,
params: {
partial: true,
}
}
The code is:
user = await User.create({
// name: 'Michael',
// id: '22',
email: '[email protected]',
address: {
street: '42 Park Ave',
state: 'WA',
zip: 98102,
},
version: 44
}, {log: true})
Ok fixed the infer type property check and also added tests so we don't have regression.
Thank you. I've got some manual merges to do here. Will post back status in a few days.