-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Feature - replaced IdSchema with FieldPathId #4787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
In order to support an upcoming feature as well as eliminate a performance issue, `IdSchema` was replace with `FieldPathId`
- In @rjsf/utils:
- Added new `FieldPathList` and `FieldPathId` types and `DEFAULT_ID_PREFIX` and `DEFAULT_ID_SEPARATOR` to `constants.ts`
- Added the new `toFieldPathId()` function to generate `FieldPathId`s, exporting it from the library
- BREAKING CHANGES
- Removed the `IdSchema` type, replacing `idSchema: IdSchema<T>` in all types with `fieldPathId: FieldPathId`
- Updated the `idGenerators` to replace `id: IdSchema<T> | string` with `id: FieldPathId | string` removing the need for the `<T = any>` generic on the functions
- Removed the `toIdSchema()` function in the `schema` directory
- Updated the `SchemaUtilsType` and `createSchemaUtils()` to remove the `toIdSchema()` function
- Deleted the `ui:rootFieldId` from the `UiSchema` since `idPrefix` does the same exact thing
- Updated the tests accordingly
- In `@rjsf/antd`, `@rjsf/chakra-ui`, `@rjsf/fluent-ui`, `@rjsf/mantine`, `@rjsf/mui`, `@rjsf/primereact`, `@rjsf/react-bootstrap`, `@rjsf/semantic-ui` and `@rjsf/shadcn`:
- BREAKING CHANGES - Updated all of the templates and widgets to change `idSchema` to `fieldPathId` or to remove the `<T>` off of the idGenerator functions
- In @rjsf/core:
- BREAKING CHANGES
- Updated all of the fields, templates and widgets to change `idSchema` to `fieldPathId` or to remove the `<T>` off of the idGenerator functions
- `ObjectField` and `ArrayField` to use `toFieldPathId` instead of `toIdSchema()` to generate the `fieldPathId`s of all its children
- Updated the `onChange` handling of fields to make `path` required and either pass it straight through, or use the `fieldPathId.path` instead of using an empty array or appending path information
- Updated `Form` to use `toFieldPathId()` to generate `fieldPathId` instead of `idSchema`, always providing the `idPrefix` and `idSeparator` in the `globalFormOptions` and make the `path: FieldPathList` required
- Updated `LayoutGridField` to remove the `IdSchema` related code in favor of `FieldPathId` code
- Also exported the `getTestRegistry()` function from the main `index.ts` to assist developers in creating `registry` object for tests
- Updated all of the test to deal with the `idSchema` -> `fieldPathId` changes
- In @rjsf/daisyui:
- BREAKING CHANGES - Updated all of the templates and widgets to change `idSchema` to `fieldPathId` or to remove the `<T>` off of the idGenerator functions
- Also fixed the `FieldTemplate` to extract the `description` element so that it was not spread onto the `div`, fixing the snapshots
- In @rjsf/validator-ajv8:
- Updated the test to no longer try to test the delete `toIdSchema` function
- In docs:
- Updated `custom-templates.md`, `custom-widgets-fields.md` and `layout-grid.md` to change the `idSchema` documentation to `fieldPathId`
- Updated `uiSchema.md` to remove the `ui:rootFieldId` documentation
- Updated `utility-functions.md` delete `toIdSchema()`, add `toFieldPathId()` and to remove the `<T>` from the id generator functions
- Updated `v6.x upgrade guide.md` to document all the breaking changes made in this release
- Updated the `CHANGELOG.md` accordingly
e2c246a to
cc5b4f9
Compare
| displayLabel, | ||
| classNames, | ||
| // Destructure props we don't want to pass to div | ||
| description, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice subtle fix, the only snapshot changes are related to this (so the ID Schema replacement is clean! 🙌 )
| uiSchema: { ...gridFormUISchema, [UI_GLOBAL_OPTIONS_KEY]: globalUiOptions }, | ||
| formData: {}, | ||
| errorSchema: { employment: {} }, | ||
| // IdSchema is weirdly recursive and it's easier to just ignore the error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not anymore!
| idPrefix: props.idPrefix || DEFAULT_ID_PREFIX, | ||
| idSeparator: props.idSeparator || DEFAULT_ID_SEPARATOR, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we able to call this.getDefaultRegistry().globalFormOptions here to get the prefix/separator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it blew things up because this is called in the constructor, before the this.props has been setup properly.
| unset(copiedFormData, key); | ||
| // drop property will pass the name in `path` array | ||
| onChange(copiedFormData, []); | ||
| onChange(copiedFormData, fieldPathId.path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to ensure no regression in #4763 since we didn't add a test for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I manually tested additional properties and arrays in my local playground. All works
Reasons for making this change
In order to support an upcoming feature as well as eliminate a performance issue,
IdSchemawas replace withFieldPathIdFieldPathListandFieldPathIdtypes andDEFAULT_ID_PREFIXandDEFAULT_ID_SEPARATORtoconstants.tstoFieldPathId()function to generateFieldPathIds, exporting it from the libraryIdSchematype, replacingidSchema: IdSchema<T>in all types withfieldPathId: FieldPathIdidGeneratorsto replaceid: IdSchema<T> | stringwithid: FieldPathId | stringremoving the need for the<T = any>generic on the functionstoIdSchema()function in theschemadirectorySchemaUtilsTypeandcreateSchemaUtils()to remove thetoIdSchema()functionui:rootFieldIdfrom theUiSchemasinceidPrefixdoes the same exact thing@rjsf/antd,@rjsf/chakra-ui,@rjsf/fluent-ui,@rjsf/mantine,@rjsf/mui,@rjsf/primereact,@rjsf/react-bootstrap,@rjsf/semantic-uiand@rjsf/shadcn:idSchematofieldPathIdor to remove the<T>off of the idGenerator functionsidSchematofieldPathIdor to remove the<T>off of the idGenerator functionsObjectFieldandArrayFieldto usetoFieldPathIdinstead oftoIdSchema()to generate thefieldPathIds of all its childrenonChangehandling of fields to makepathrequired and either pass it straight through, or use thefieldPathId.pathinstead of using an empty array or appending path informationFormto usetoFieldPathId()to generatefieldPathIdinstead ofidSchema, always providing theidPrefixandidSeparatorin theglobalFormOptionsand make thepath: FieldPathListrequiredLayoutGridFieldto remove theIdSchemarelated code in favor ofFieldPathIdcodegetTestRegistry()function from the mainindex.tsto assist developers in creatingregistryobject for testsidSchema->fieldPathIdchangesidSchematofieldPathIdor to remove the<T>off of the idGenerator functionsFieldTemplateto extract thedescriptionelement so that it was not spread onto thediv, fixing the snapshotstoIdSchemafunctioncustom-templates.md,custom-widgets-fields.mdandlayout-grid.mdto change theidSchemadocumentation tofieldPathIduiSchema.mdto remove theui:rootFieldIddocumentationutility-functions.mddeletetoIdSchema(), addtoFieldPathId()and to remove the<T>from the id generator functionsv6.x upgrade guide.mdto document all the breaking changes made in this releaseCHANGELOG.mdaccordinglyChecklist
npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:updateto update snapshots, if needed.