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

Fixes session type as any when using .keystone/types Lists as your root type #8743

Merged
merged 7 commits into from
Aug 8, 2023

Conversation

dcousens
Copy link
Member

@dcousens dcousens commented Aug 8, 2023

This pull request fixes session type becoming any when using .keystone/types Lists as your root type for your lists definition.

@changeset-bot

This comment was marked as resolved.

@dcousens dcousens self-assigned this Aug 8, 2023
@dcousens dcousens requested a review from borisno2 August 8, 2023 02:26
@dcousens dcousens added the 🐛 bug Unresolved bug label Aug 8, 2023
@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 8, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9a6a0db:

Sandbox Source
@keystone-6/sandbox Configuration

@dcousens dcousens merged commit aef0bf3 into main Aug 8, 2023
@dcousens dcousens deleted the session-fixed branch August 8, 2023 04:37
@@ -194,7 +194,7 @@ export function printGeneratedTypes(
prismaClientPath = stringify(prismaClientPath).replace(/'/g, `\\'`);

for (const [listKey, list] of Object.entries(lists)) {
const listTypeInfoName = `Lists.${listKey}.TypeInfo`;
const listTypeInfoName = `Lists.${listKey}.TypeInfo<Session>`;
Copy link
Contributor

Choose a reason for hiding this comment

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

@dcousens Unfortunately this change leads to a TSC error in our system where we have 153 models. TSC is unable to do typechecking with this error:

RangeError: Maximum call stack size exceeded

Copy link
Contributor

Choose a reason for hiding this comment

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

removing the passed in Session generic solves the above issue:

-   ...
-    readonly User: Lists.User.TypeInfo<Session>;
-   ...
+   ...
+    readonly User: Lists.User.TypeInfo;
+   ...

Copy link
Member Author

@dcousens dcousens Aug 8, 2023

Choose a reason for hiding this comment

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

@mmachatschek interesting, could you tell me anything about the shape of your Session type?
And or maybe how "interconnected" your models are?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dcousens the Session type has several fields but no more than 2 levels of data. All the fields of the Session object have string | null or object | null types.

The models itself very interconnected. As there is a lot of business logic I can not share much of the details or screenshots of the schema/models :/

Copy link
Contributor

@mmachatschek mmachatschek Aug 9, 2023

Choose a reason for hiding this comment

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

More observations so far.

If I add the <Session> generics to a subset of the models, the call stack size doesn't exceed. My guess, the amount of models and therefor the amount of type checking needed is just too big

Edit: So the generic on a specific list/model is not the culprit. Its just the amount of models

Copy link
Contributor

Choose a reason for hiding this comment

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

@dcousens I found a feasible solution for us (increasing the v8 stack size for type checking):

keystone disables type checking for keystone build so its ok in this case to ignore this error 🤞 with some hope this is better implemented in a future version of typescript

node --stack-size=4000 ./node_modules/typescript/lib/tsc.js

Copy link
Member Author

@dcousens dcousens Aug 9, 2023

Choose a reason for hiding this comment

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

I will try and produce a reproduction, conceptually if it's only the number of lists and the number of relationships therein, this shouldn't be too difficult to reproduce.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tracking this in #8749

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Unresolved bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants