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

Create separate generic for schema definition #13856

Closed
wants to merge 6 commits into from

Conversation

vkarpov15
Copy link
Collaborator

Summary

Re: #13523, #13772.

The current implementation of automatic type inference makes it tricky for us to automatically infer the correct hydrated document type. Because automatic type inference infers the DocType, which is a mix of the hydrated document type and the raw doc type: hydrated for arrays and maps, raw for everything else, including nested paths. And then we infer the schema definition type from the DocType, which means we lose whether object paths are nested paths or subdocument paths.

This PR presents one approach that can help us move towards automatically inferring the raw doc type and the hydrated doc type correctly: getting the schema definition and inferring the raw doc type from that. In theory, if we have the schema definition type, we should be able to infer the hydrated document type from that.

The downside is this approach seems to require adding as const to schema definitions if you want to use type: 'String' or type: 'Number' as opposed to type: String or type: Number. Also has some issues with context for default functions, but those issues are present to some extent even without this PR and have easy workarounds.

cc @mohammad0-0ahmad

Examples

@mohammad0-0ahmad
Copy link
Contributor

Hello 👋

Sorry for being inactive lately, I will try to find time to get more involved again.

Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK, it is really not ideal to require explicit casting or redundant as const in some places, as long as it is limited to the string representation (of primitives) it should be OK

i have not yet tested this change (or 8.0 in general) against typegoose for type issues

test/types/schema.test.ts Outdated Show resolved Hide resolved
@@ -95,14 +95,15 @@ async function autoTypedVirtuals() {
const testSchema = new Schema({
email: {
type: String,
required: [true, 'email is required']
required: [true, 'email is required'] as [true, string]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are those array castings really required instead of as const?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately yes. I think that's more of a TypeScript 5 issue though, similar to #13514.

test/types/virtuals.test.ts Show resolved Hide resolved
@vkarpov15
Copy link
Collaborator Author

I'm going to close this PR for now, and postpone the task of automatically inferring hydrated document type to a future release. There's just too many surprise test issues related to this PR that I am unable to understand, so my confidence level in this PR is low. We'll leave this branch as an informational branch for future work.

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.

3 participants