List enum values should be typed as strings #108
Conversation
|
Hi @valtyr are you still working on prisma-kysely? Appreciate your time is precious especially in open source, so thank you for your hard work. Would you be looking for maintainers to help out manage prisma-kysely? Would love to see this PR merged and released if you think it's a good addition. Cheers |
|
Hey @alii, I'm sorry for the late reply. Nice work on this PR! Sadly, prisma-kysely has been on the backburner recently cause of work stuff... but thankfully there's not a lot of maintenance that needs to happen here in general. Seems to me like the code all makes sense. Would be be so kind as to adding a changeset with a minor version bump, that explains that this version includes breaking changes to the way we handle arrays of enums in Postgres. That way we can get this merged and deployed ASAP. I'm open to having a co-maintainer on the project. Is that something that you would be interested in? If so we could have a chat about things on Discord, just let me know. |
| "prisma-kysely": minor | ||
| --- | ||
|
|
||
| Make enum array values actually be strings |
There was a problem hiding this comment.
Haha, please update this so it mentions that this might have breaking behavior for people that have added their own array parsers etc.
| provider = "sqlite" | ||
| url = "file:./dev.db" | ||
| provider = "postgresql" | ||
| url = "env(DATABASE_URL)" |
There was a problem hiding this comment.
I think this shouldn't actually work in CI since we're not running a PG server there. Either we skip this in the e2e test or we need to set up a PG server to test against. The server URL should be hard-coded in the test as well so it's deterministic.
There was a problem hiding this comment.
Got it - the reason I changed it is because enum arrays are not supported in SQLite so prisma wouldn't generate the schema during the test run. Will have more of a think about this. Perhaps skipping it would be ok
| @@ -63,5 +63,6 @@ | |||
| ], | |||
| "importOrderSeparation": true, | |||
| "importOrderSortSpecifiers": true | |||
| } | |||
| }, | |||
| "packageManager": "[email protected]+sha1.4ba7fc5c6e704fce2066ecbfb0b0d8976fe62447" | |||
There was a problem hiding this comment.
Will remove, was added automatically by corepack. Worth noting though that locking down the package manager (and subsequently having corepack automatically use the right package manager version) is not a bad idea - perhaps we can re-add in another PR?
No worries, must reiterate how amazing the work is you've done so far, so really a big thanks. Regards to co-maintaining, I would be happy to do that! What's your Discord? Will get to making these adjustments asap, thanks again. |
Haha thanks, I'm just happy that my code can be useful to others. It would be awesome if you'd be willing to consider joining as a co-maintainer. Let's find time to chat. My username is |
| ), | ||
| ] | ||
| ) | ||
| : ts.factory.createTypeReferenceNode( |
There was a problem hiding this comment.
this ts.factory.createTypeReferenceNode( can also be reused above to improve readability.
| export type Timestamp = ColumnType<Date, Date | string, Date | string>;`; | ||
| export type Timestamp = ColumnType<Date, Date | string, Date | string>; | ||
| export type EnumArrayInner<T extends string, All extends string> = T extends infer Single extends string ? T | \`\${Single},\${Exclude<All, Single>}\` : never | ||
| export type EnumArray<T extends string> = '{}' | \`{\${EnumArrayInner<T, T>}}\`;`; |
There was a problem hiding this comment.
having this always would throw ts error for unused variable, maybe would be good to add this conditionaly.
This PR introduces a fix where enum list values are typed as
MyEnum[]but Prisma's migrations actually store them as a string format.Since kysely is just a query builder, we couldn't expect it to parse this string format into an array, so I think it makes the most sense to have it typed like this.
If you would like to merge this, please could I ask you throughly check the PR, it's very possible I've missed something as I only spent a few minutes looking around the codebase for supporting this feature.
Thanks for prisma-kysely, it's a wonderful tool!
fixes #107