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

Fix composite type declaration with a "type" composite fragment #484

Merged
merged 4 commits into from
Nov 8, 2023

Conversation

kfirba
Copy link
Contributor

@kfirba kfirba commented Nov 7, 2023

Hey.

I have a pivot table with a composite key, where a fragment of the primary key is an enum:

create type "TopupType" as enum('A', 'B', 'C', 'D', 'E', 'F');

create table "Topups" (
  "someOtherTableId" text not null references "SomeOtherTable" ("id") on delete cascade on update restrict,
  "type" "TopupType" not null,
  -- ...
  primary key ("someOtherTableId", "type")
);

Without this PR, there's a faulty generated type for this table:

export const topupsType: z.Schema<TopupsType> = undefined as any;

It happens since the getIdentifierDeclaration function has no awareness of other non-composite types.
This PR falls back to inspect the nonCompositeTypeImports object in case the composite type is not part of the built-in type mappings.

The result now is:

export const topupsType: z.Schema<TopupsType> = topupType as any;

@kristiandupont
Copy link
Owner

Hi @kfirba, thank you for the contribution! There are some linter errors, if you fix those I will merge it :-)

@kfirba
Copy link
Contributor Author

kfirba commented Nov 8, 2023

@kristiandupont The linting issues are not from the change I made, they are there in main, I suppose.
I can take a look

@kristiandupont
Copy link
Owner

Ah, then perhaps it's because I updated some dependencies recently. CI should have caught that but I tend to merge many at a time so sometimes things slip through..

@kristiandupont
Copy link
Owner

Indeed the problem is on main. I'm fixing it.

@kfirba
Copy link
Contributor Author

kfirba commented Nov 8, 2023

@kristiandupont Great :)
Let me know when it's in so I can rebase and update the PR

@kristiandupont
Copy link
Owner

Try now!

@kfirba
Copy link
Contributor Author

kfirba commented Nov 8, 2023

@kristiandupont Should be good now :)

@kristiandupont kristiandupont merged commit a58d19d into kristiandupont:main Nov 8, 2023
1 check passed
@kristiandupont
Copy link
Owner

Merged and published. Thank you for contributing, nice find!

@kfirba
Copy link
Contributor Author

kfirba commented Nov 8, 2023

Thanks for the package!

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