export AutoFormProps and AutoTableProps for auto/polaris #807
Conversation
There was a problem hiding this comment.
- 🍏 Types getting exported for the Form and Table look reasonable
- 🟡 It feels weird that you can pass in
recordanddefaultValuesat the same time given they do similar things in slightly different ways. Perhaps the type should make these mutually exclusive.
I'm gonna come back to this with fresh eyes to test deeply selected records in deep AutoRelationshipForms.
packages/react/src/auto/AutoForm.ts
Outdated
| /** A record for this form to act on */ | ||
| record?: GadgetRecord<any>; |
There was a problem hiding this comment.
This comment contradicts the above comment saying
This action doesn't run against existing records, so you can't pass a findBy option
I'm realizing now that this record prop clashes with the defaultValues prop for prepopulating the form values. Should we deprecate one of them to avoid confusion?
Also I think it'd be better to have a action input dependent type passed into the GadgetRecord. I tried doing something like that for defaultValues previously, but it breaks for relationships where the type is based on nested actions like belongsToField: {create: {.... I think that would need another API client core change. That could be considered separately tho
| import { testApi as api } from "../../../apis.ts"; | ||
|
|
||
| const AutoFormWithExistingRecord = () => { | ||
| const [{ data: widget }] = useFindOne(api.widget, "1145"); |
There was a problem hiding this comment.
Interesting
When this fetch has a select: {...} that does not include all of the fields in the form, the unselected fields will get nullified in the submit.
I think this is correct because the resulting record matches the state of the form. Not selecting in the fetch is basically a reset field operation
infiton
force-pushed
the
export-auto-props
branch
from
38f8b75 to
fe78656
Compare
infiton
force-pushed
the
export-auto-props
branch
from
fe78656 to
93e03c8
Compare
| export type { PolarisAutoFormProps as AutoFormProps } from "./PolarisAutoForm.js"; | ||
| export { PolarisAutoTable as AutoTable } from "./PolarisAutoTable.js"; | ||
| export type { PolarisAutoTableProps as AutoTableProps } from "./PolarisAutoTable.js"; |
There was a problem hiding this comment.
recordprop- 🍏 Passing in a record without
idselected makes an error - 🍏
defaultValueswins overrecordfor root level fields - 🟡 Passing in a record with hasMany children does not load them in the form
- I suspect that
...edges.node...not getting translated properly to the form state is the reason. I think this could be followed up though as unblocking Nick feels more imporatant
- I suspect that
- 🍏 Passing in a record without
packages/react/src/auto/shadcn/unreleasedIndex.ts
^ in here, it'd be nice to add the type exports aswell for the Shadcn side so we don't gotta remember to do it later
| : // eslint-disable-next-line @typescript-eslint/ban-types | ||
| { | ||
| /** This action doesn't run against existing records, so you can't pass a findBy option */ | ||
| findBy?: never; | ||
| /** This action doesn't operate with a record, so you can't pass a record option */ | ||
| record?: never; |
There was a problem hiding this comment.
I think this can be safely removed. When it's here, the record prop still appears in the autocomplete dropdown like this.
| throw new Error("The 'findBy' prop is required for actions that operate with a record identity."); | ||
| } else if (!operatesWithRecordId && hasFindBy) { | ||
| throw new Error("The 'findBy' prop is only allowed for actions that operate with a record identity."); | ||
| if (record) { |
There was a problem hiding this comment.
Having record and findBy at the same time should probably throw an error. When that happens, the findBy lookup is paused because record is given, and record.id will be used in the submit request regardless of what findBy is
| if (record) { | ||
| if (operatesWithRecordId && !record.id) { | ||
| throw new Error("Passing a record to an action that operates with a record identity requires the record to have an id."); | ||
| } else if (!operatesWithRecordId && record.id) { |
There was a problem hiding this comment.
There is a type error for having the record prop for non-id actions. Perhaps this should throw when there's a record instead of a record that has an ID
@pistachiobaby has run into an issue when generating reference cards for AutoForm and AutoTable that the props type for these are not exported; this PR adds these as exports
I also changed the type of AutoForm props that make it clear that for actions that take record identity that either a findBy or record prop is required. I also added some more test coverage for passing a record instead of a findBy
cc @airhorns
PR Checklist