feat: support for explicit column names for references #1458
Conversation
|
|
||
| export type Name = string | { schema?: string; name: string }; | ||
|
|
||
| export type Reference = Name & { |
There was a problem hiding this comment.
question: I assume the & is only useful when Name is not a string? Should we split the types here in a better way?
e.g.
export type NameSchema = { schema?: string; name: string }
export type Name = string | NameSchema;
export type Reference = (NameSchema & { columns?: string | string[] }) | Name;
additional question: when having a columns, is schema required? or only-always name?
Maybe we could even do
type Name_ = string;
export type NameSchema = { schema?: string; name: Name_ }
export type NameSchemaWithColumn = NameSchema & { columns?: string | string[] }
export type Reference = Name | NameSchema | NameSchemaWithColumn;
export type Name = Name_ | NameSchema; // don't make breaking change for exposed API
There was a problem hiding this comment.
All in all I tried to do quite conservative changes, so I did not take the liberty to do more changes. I agree though that it would be better if it was an explicit object when extending with columns. And no, schema should not be required as it wasn't before either, so we will not do any backwards breaking changes.
src/operations/tables/shared.ts
Outdated
| const columns: string[] = ( | ||
| references.columns == null ? [] : [...toArray<string>(references.columns)] | ||
| ) | ||
| .filter((column: string) => column && literal) |
There was a problem hiding this comment.
question: isn't literal always defined?
There was a problem hiding this comment.
There were some failing unit tests, so it appears that there are some cases in which it wasn't always defined
| import type { Name, Reference, Value } from '../operations/generalTypes'; | ||
|
|
||
| export type Literal = (v: Name) => string; | ||
| export type Literal = (v: Name | Reference) => string; |
There was a problem hiding this comment.
I think Reference includes Name already, does it?
There was a problem hiding this comment.
Yes it does, 100%, but again I thought of conservative changes, since Literal is used in other places too. In other words if Name is usually passed as an argument it might be more confusing to see my new Reference type.
| references: { | ||
| schema: 'otherSchema', | ||
| name: 'otherTable', | ||
| columns: ['colC', 'colD'], |
There was a problem hiding this comment.
question: Did we missed to add string[] as possibility for References.columns type?
There was a problem hiding this comment.
Sorry, I did not understand the question, unless you meant that before this it wasn't possible to define columns explicitly. Please elaborate if I did not understand. :)
Co-authored-by: Shinigami <chrissi92@hotmail.de>
|
@adrenalin please rebase |
|
We are currently planning to release a new major. Do you want to work on this PR so it will be included? Or do you want to have some more time and then we might release this in a later version? |
Sorry, I hadn't seen your comment before! It depends on when you are planning on releasing new version, but by default yes I would like to have this included. I should have time to work on this within the coming days. |
|
Okay, I plan to include this in the full release of the next major, so I think I will release one or several alphas in between. |
|
@adrenalin any news? I have not released a v9 non-alpha yet, so it's still possible that this could land in v9. However you need to fix the conflicts and make the CI green. |
2 participants
I found it confusing that I cannot explicitly define the foreign key column names in any other way than providing a string, so I created a new Reference type that extends Name by adding optional columns.
I also changed the Type section of the documentation to Types (deliberately the last commit), which makes this look like a lot bigger change than it really is, because
pnpm preflightdoes a lot of formatting to keep the markdown tables coherent. Please do let me know if this change is not welcome.