Conversation
yyyyaaa
changed the title
| if (depth > 3 || fields.length === 0) { | ||
| // Use first field if available, otherwise fallback to 'id' | ||
| const fallbackName = fields.length > 0 ? fields[0].name : 'id'; | ||
| return t.objectExpression([t.objectProperty(t.identifier(fallbackName), t.booleanLiteral(true))]); |
There was a problem hiding this comment.
I think we should maybe just remove defaults. I think it could be trouble to assume id, and why not just force people to select?
There was a problem hiding this comment.
I think this can get us into trouble, also removing buildDefaultSelectExpression and not doing defaults can simplify the code.
I know that it will make our current migration a bit more hard, but I think this type of thing feels like it can make code harder to maintain, and also maybe some surprising issues like id doesn't exist...
| const pkField = pkFields[0]; | ||
| const pkFieldName = pkField?.name ?? 'id'; | ||
| const pkFieldTsType = pkField?.tsType ?? 'string'; | ||
|
|
There was a problem hiding this comment.
what happens if there is no primary key? I know it's a rare case.
| const patchTypeName = `${typeName}Patch`; | ||
|
|
||
| const pkFields = getPrimaryKeyInfo(table); | ||
| const pkField = pkFields[0]; |
There was a problem hiding this comment.
what if there is no pk field?
| ): { document: string; variables: Record<string, unknown> } { | ||
| const entitySelections = select | ||
| ? buildSelections(select as Record<string, unknown>, connectionFieldsMap, operationName) | ||
| : [t.field({ name: 'id' })]; |
There was a problem hiding this comment.
another assumption that we have an id
| ): { document: string; variables: Record<string, unknown> } { | ||
| const selections = select | ||
| ? buildSelections(select as Record<string, unknown>, connectionFieldsMap, operationName) | ||
| : [t.field({ name: 'id' })]; |
There was a problem hiding this comment.
assumption that we have an id
| ): { document: string; variables: Record<string, unknown> } { | ||
| const entitySelections = select | ||
| ? buildSelections(select as Record<string, unknown>, connectionFieldsMap, operationName) | ||
| : [t.field({ name: 'id' })]; |
There was a problem hiding this comment.
assumption that we have id
| lines.push(`import type { UseMutationOptions, UseMutationResult } from '@tanstack/react-query';`); | ||
| lines.push(`import { getClient } from '../client';`); | ||
| lines.push(`import { buildSelectionArgs } from '../selection';`); | ||
| lines.push(`import type { SelectionConfig } from '../selection';`); |
There was a problem hiding this comment.
AST to string regression
| lines.push(` const args = buildSelectionArgs<${selectTypeName}>(params?.selection);`); | ||
| lines.push(` const { selection: _selection, ...mutationOptions } = params ?? {};`); | ||
| lines.push(` void _selection;`); | ||
| lines.push(` return useMutation({`); |
There was a problem hiding this comment.
another regression — let's make sure these are AST
|
|
||
| lines.push(` ...mutationOptions,`); | ||
| lines.push(` });`); | ||
| lines.push(`}`); |
There was a problem hiding this comment.
more regressions up until here, needs to be 100% AST based.
| reactQueryTypeImport.importKind = 'type'; | ||
| statements.push(reactQueryTypeImport); | ||
| lines.push(`import { useQuery } from '@tanstack/react-query';`); | ||
| lines.push(`import type { UseQueryOptions, UseQueryResult, QueryClient } from '@tanstack/react-query';`); |
| lines.push(`import type { SelectionConfig } from '../selection';`); | ||
|
|
||
| if (useCentralizedKeys) { | ||
| lines.push(`import { customQueryKeys } from '../query-keys';`); |
|
closed in favor of Dan's PR with my fixes on top |
2 participants
Testing a bit more before merging but all tests passing..