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

perf: speed up addQuestionMarks #2620

Closed
wants to merge 2 commits into from

Conversation

Andarist
Copy link

Just an experiment to speed up addQuestionMarks. I wonder if the change is 100% semantically the same and what your CI might tell us about it.

@netlify
Copy link

netlify bot commented Jul 31, 2023

Deploy Preview for guileless-rolypoly-866f8a ready!

Name Link
🔨 Latest commit 936900b
🔍 Latest deploy log https://app.netlify.com/sites/guileless-rolypoly-866f8a/deploys/661e0a9e73667d0008f8b34a
😎 Deploy Preview https://deploy-preview-2620--guileless-rolypoly-866f8a.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

[k in keyof T]: undefined extends T[k] ? never : k;
}[keyof T];
type keepRequiredKeys<T extends object> = {
[k in keyof T as undefined extends T[k] ? never : k]-?: T[k];
Copy link
Author

Choose a reason for hiding this comment

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

if some semantic difference is found using this approach then we could also try this

Suggested change
[k in keyof T as undefined extends T[k] ? never : k]-?: T[k];
[k in keyof T as undefined extends T[k] ? never : k]-?: unknown;

@colinhacks
Copy link
Owner

colinhacks commented Apr 16, 2024

@Andarist Thanks for this! I only recently added in some decent tests for addQuestionMarks. The behavior of Zod in userland generic functions is very fiddly and dependent on subtle details in the addQuestionMarks implementation.

TLDR: This is a very clever approach but I couldn't get it to play nice with the current set of tests (mostly added a few weeks ago) in generics.test.ts and object.test.ts. You might be mildly interested by failing test cases! Here's one that's surprising tricky to pass (from generics.test.ts):

test("assignability", () => {
  const createSchemaAndParse = <K extends string, VS extends z.ZodString>(
    key: K,
    valueSchema: VS,
    data: unknown
  ) => {
    const schema = z.object({
      [key]: valueSchema,
    });
    const parsed = schema.parse(data);
    const inferred: z.infer<z.ZodObject<{ [k in K]: VS }>> = parsed;
    return inferred;
  };
  createSchemaAndParse("foo", z.string(), { foo: "" });
});

It seems the main issue with your keepRequiredKeys approach is that there's no longer an assertion that the keys of the resulting object extend keyof T due to the as clause. When writing to do inference or type manipulation inside generic functions, that seems to be important 🤷‍♂️

For reference here's the current implementation:

type optionalKeys<T extends object> = {
  [k in keyof T]: undefined extends T[k] ? k : never;
}[keyof T];
type requiredKeys<T extends object> = {
  [k in keyof T]: undefined extends T[k] ? never : k;
}[keyof T];
type pickRequired<T extends object, R extends keyof T = requiredKeys<T>> = {
  [k in R]: T[k];
};
type pickOptional<T extends object, O extends keyof T = optionalKeys<T>> = {
  [k in O]?: T[k];
};
export type addQuestionMarks<T extends object> = pickRequired<T> &
  pickOptional<T> & { [k in keyof T]?: unknown };

@Andarist
Copy link
Author

It seems the main issue with your keepRequiredKeys approach is that there's no longer an assertion that the keys of the resulting object extend keyof T due to the as clause.

It should understand this but:

  • it could depend on TS version
  • I wouldn't be surprised that something might stay unhandled on TS side of things here

I'm interested in learning more about this but I also don't have time to spend on this... what to do, what to do 😭

@colinhacks colinhacks closed this Apr 23, 2024
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.

2 participants