Closed gittgott closed 4 months ago
Great start! I looked at the code and it looks quite good. Two comments:
I should definitely add an ESLint auto-fix rule for this, but until then, can you keep the properties alphabetically sorted? 😁
I don't think we should reuse IdentifierNode
for this. I would introduce RawExpressionNode
like this (and add it to ExpressionNode
and NodeType
:
import { NodeType } from './node-type';
export class RawExpressionNode {
readonly expression: string;
readonly type = NodeType.RAW_EXPRESSION;
constructor(expression: string) {
this.expression = expression;
}
}
In the future, I guess the Overrides.columns
signature will look like Record<string, Node | string>
. If it's not an easy fix, we could skip it for now. I appreciate the work!
I updated the column.overrides to allow for ExpressionNode
s instead of just strings. This appears to be working for me with something like the following:
{
overrides: {
columns: {
"<table_name>.<column_name>": new GenericExpressionNode("Generated", [
new RawExpressionNode("boolean"),
]),
},
},
};
Looks great! I will test this as soon as I have time.
I have one more fix that I just committed.
I remembered that kysely prefers json columns to be wrapped with the JSONColumnType
type which is really just an alias for ColumnType<T, string, string>
. This is due to the fact that the data should be a string when updating and inserting to that column, but will come back as JSON when selecting.
I added a node just like ColumnType
called JSONColumnType
to simplify the usage of JSON columns in overrides.
I wanted to be able to directly import JSONColumnType from kysely in the generated code, but adding that to GLOBAL_IMPORTS
didn't seem to be working - it wasn't including that as an import in the generated file. Because of this, I just am outputting ColumnType<T, string, string>
since ColumnType
is already imported.
All of this is possible by just using a string override (like { '<column_name>.<table_name>': 'ColumnType<{ postalCode: string; street: string; city: string }>, string, string' }
) but I think that that could be a bit confusing to write.
is it possible to use this from cli?
is it possible to use this from cli?
see pr here: https://github.com/gittgott/kysely-codegen/pull/2
I have one more fix that I just committed.
I remembered that kysely prefers json columns to be wrapped with the
JSONColumnType
type which is really just an alias forColumnType<T, string, string>
. This is due to the fact that the data should be a string when updating and inserting to that column, but will come back as JSON when selecting. I added a node just likeColumnType
calledJSONColumnType
to simplify the usage of JSON columns in overrides. I wanted to be able to directly import JSONColumnType from kysely in the generated code, but adding that toGLOBAL_IMPORTS
didn't seem to be working - it wasn't including that as an import in the generated file. Because of this, I just am outputtingColumnType<T, string, string>
sinceColumnType
is already imported.All of this is possible by just using a string override (like
{ '<column_name>.<table_name>': 'ColumnType<{ postalCode: string; street: string; city: string }>, string, string' }
) but I think that that could be a bit confusing to write.
gittgott, I noticed GLOBAL_IMPORTS
are not included in the output without a corresponding matching node. When it didn't work for me, I tried to import a TypeScript namespace, Temporal
, and utilize with Temporal.PlainDate
. Excluded as Temporal != Temporal.PlainDate
.
Amazing work guys! I implemented both your suggestions and also made the Kysely JSONColumnType
work. This will be in the next release as soon as I release it.
This is somewhat related to #34, #75, and #30.
This follows the format in the JSON configuration issue (#34), so it should be adaptable to that. This does not implement any sort of TypeScript parsing. It will simply copy the value of the column name's override as an
IdentifierNode
. This will at least allow for simple type overrides, but I'm not entirely certain of what will and won't be supported with this.I was also uncertain if the override in
#transformColumn
should truly override the type or if it should respect theisNullable
orisGenerated
parts of that. For now, I just have it as a true override which ignores those.I have kept this as a property on the
Generator
class, and it is not included as a CLI option. I was not sure how this would be best implemented, so I left it alone for now and am just making it available through custom scripts.I'm looking for some feedback on my prior uncertainties and if I'm missing anything glaringly obvious. Thanks!