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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

add type constraints for various sql fragment functions #8688

Merged
merged 1 commit into from Jan 30, 2024

Conversation

b0b3rt
Copy link
Collaborator

@b0b3rt b0b3rt commented Jan 24, 2024

Adds some generic type constraints to various functions in the parent branch, so as to make life a bit easier & prevent typos/etc.

The main thing I'm not sure about here is the type of on in SqlJoinBase: Partial<Record<FieldName<N>, string>> technically allows { _id: undefined }. (The previous Record<string, string> did not.)

There isn't an easy way that I can find to represent a generic type constraint which allows an arbitrary selection of keys from a union type, where the value of those keys must not be undefined if the key is in the object (but any individual key is not required to be in the object). microsoft/TypeScript#13195 introduced --strictOptionalProperties but we can't enable that; too much stuff breaks.

I think it's worth the trade-off here but 馃し

鈹咺ssue is synchronized with this Asana task by Unito

@b0b3rt b0b3rt requested a review from a team as a code owner January 24, 2024 22:16
@b0b3rt b0b3rt requested review from Will-Howard and removed request for a team January 24, 2024 22:16
@s-cheng s-cheng requested review from oetherington and removed request for Will-Howard January 29, 2024 21:02
Copy link
Collaborator

@oetherington oetherington left a comment

Choose a reason for hiding this comment

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

This is much nicer - thanks!

@@ -279,7 +280,7 @@ class ProjectionContext<N extends CollectionNameString = CollectionNameString> {
private compileJoin({table, type = "inner", on, prefix}: SqlJoinSpec): string {
const selectors: string[] = [];
for (const field in on) {
selectors.push(`"${prefix}"."${field}" = ${on[field]}`);
selectors.push(`"${prefix}"."${field}" = ${on[(field as keyof typeof on)]}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's pretty sad that TS can't infer this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah :(

@oetherington oetherington merged commit ef61491 into sql-fragments Jan 30, 2024
12 checks passed
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.

None yet

3 participants