Upgrade to RJSF v5 #1819

nickgros · 2022-10-05T19:48:20Z

Upgrade to RJSF v5.

Includes many changes to clean up code + fix existing bugs.

Tests still failing.

Changes to fix tests and build errors

nickgros · 2022-10-12T18:14:06Z

esbuild.config.mjs

@@ -15,7 +15,6 @@ const globals = {
  'rss-parser': 'Parser',
  'react-mailchimp-subscribe': 'ReactMailchimpSubscribe',
  'plotly.js-basic-dist': 'Plotly',
-  '@rjsf/core': 'JSONSchemaForm',


Let's include RJSF in the bundle. Bundle size doesn't change much since we were bundling our forked version, and now we're dropping it.

nickgros · 2022-10-12T18:15:06Z

src/__tests__/lib/containers/entity/annotations/SchemaDrivenAnnotationsEditor.test.tsx

-// These tests are unstable, so we'll skip them until we can fix them
-// The component is in experimental mode only, so not a big deal for now
-describe.skip('SchemaDrivenAnnotationEditor tests', () => {
+describe('SchemaDrivenAnnotationEditor tests', () => {


Hopefully these are stable now. The test suite also should emit no errors/warnings.

nickgros · 2022-10-12T18:16:42Z

src/__tests__/lib/containers/entity/annotations/SchemaDrivenAnnotationsEditor.test.tsx

@@ -191,6 +195,17 @@ describe.skip('SchemaDrivenAnnotationEditor tests', () => {
        return res(ctx.status(201), ctx.json({ token: mockAsyncTokenId }))
      },
    ),
+    rest.get(


A while ago we changed how SynapseClient requests the result for asynchronous jobs. This accommodates that change.

nickgros · 2022-10-12T18:17:13Z

src/__tests__/lib/containers/entity/annotations/SchemaDrivenAnnotationsEditor.test.tsx

+  // TODO: Test is not working, but the functionality is working in the browser
+  it.skip('Removes a custom annotation field when the last value is removed', async () => {


This is the only skipped test now.

nickgros · 2022-10-12T18:25:32Z

src/lib/containers/entity/SchemaDrivenAnnotationEditor/AnnotationEditorUtils.ts

+ * - convert schema-defined formData from an array to a non-array if the schema type is not an array
+ * @param props
+ */
+export function objectFieldComponentDidUpdate(props: FieldProps) {


This logic used to be split up in FieldTemplate and AdditionalPropertiesSchemaField. Each of those components wrapped the call to onChange in useEffect with a setTimeout.

That wasn't working very well. In short, multiple fields would call onChange at once, and only one would succeed.

This eliminates the race condition by changing all of the fields in the object in one pass.

nickgros · 2022-10-12T18:28:02Z

...lib/containers/entity/SchemaDrivenAnnotationEditor/field/AdditionalPropertiesSchemaField.tsx

-   * If the intitial data is a string, returns an array of substrings separated by commas.
-   * Otherwise, wrap the data in an array.
-   */
-  function convertToArray(value: T): Array<any> {


moved to AnnotationEditorUtils

nickgros · 2022-10-12T18:29:31Z

...lib/containers/entity/SchemaDrivenAnnotationEditor/field/AdditionalPropertiesSchemaField.tsx

-  const { list, handleListChange, handleListRemove, appendToList, setList } =
-    useListState(convertToArray(formData))
-
+  const list = Array.isArray(formData) ? formData : convertToArray(formData)


Let's remove this excess state (which also lets us get rid of another effect)

nickgros · 2022-10-12T18:29:57Z

...lib/containers/entity/SchemaDrivenAnnotationEditor/field/AdditionalPropertiesSchemaField.tsx

-  useEffect(() => {
-    // The item may not be an array when we get it, and we need to convert it right away because the order of items is not stable, and seems to depend on if the item is an array or not.
-    // Otherwise, the order of the properties will change when the user modifies the data. We may be able to fix this by modifying react-jsonschema-form to stabilize the item order.
-
-    // FIXME: This doesn't work without a delay.
-    setTimeout(() => {
-      onChange(list)
-    }, 50)
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [])


This logic was moved to ObjectField via AnnotationEditorUtils.objectFieldComponentDidUpdate

Fix test suite broken from moving files

nickgros · 2022-10-12T21:07:44Z

src/lib/containers/entity/SchemaDrivenAnnotationEditor/field/ObjectField.tsx

+/**
+ * This file is mostly a direct copy of the RJSF ObjectField component. Because it handles a lot of complex logic for
+ * rendering a form, when we upgrade RJSF, we should replace this file with the new version.
+ *
+ * The only change that we have applied is adding the `componentDidUpdate` method to the class.
+ */


See here ^^^

nickgros · 2022-10-12T21:09:34Z

src/lib/containers/entity/SchemaDrivenAnnotationEditor/template/ArrayFieldItemTemplate.tsx

+ *
+ * @param props - The `ArrayFieldTemplateItemType` props for the component
+ */
+export default function ArrayFieldItemTemplate<T, F>(


Template that didn't exist before RJSF v5. Pulled from our custom ArrayFieldTemplate.

nickgros · 2022-10-13T13:01:05Z

...ib/containers/entity/SchemaDrivenAnnotationEditor/template/ArrayFieldDescriptionTemplate.tsx

+ * Identical to @rjsf/core ArrayFieldDescriptionTemplate except doesn't render null if there is no description
+ * @param props - The `ArrayFieldDescriptionProps` for the component
+ */
+export default function ArrayFieldDescriptionTemplate<T = any, F = any>(


Template that didn't exist before RJSF v5. Mostly copied and pasted from the base one

nickgros · 2022-10-13T13:02:13Z

src/lib/containers/entity/SchemaDrivenAnnotationEditor/template/ArrayFieldTemplate.tsx

+ *
+ * @param props - The `ArrayFieldTemplateItemType` props for the component
+ */
+function ArrayFieldTemplate<T = any, F = any>(


Moved from CustomArrayFieldTemplate. Since it now uses other templates that didn't exist before, it was substantially rewritten.

nickgros · 2022-10-13T13:02:38Z

src/lib/containers/entity/SchemaDrivenAnnotationEditor/template/ArrayFieldTemplate.tsx

+  useEffect(() => {
+    if (props.items.length === 0) {
+      props.onAddClick()
+    }
+  }, [props])


This is our addition (unchanged from previous version)

nickgros · 2022-10-13T13:09:32Z

src/lib/containers/entity/SchemaDrivenAnnotationEditor/template/ArrayFieldTitleTemplate.tsx

+ * @param props
+ * @returns
+ */
+export default function ArrayFieldTitleTemplate<T, F>(


Template that didn't exist before RJSF v5.

nickgros · 2022-10-13T13:10:19Z

src/lib/containers/entity/SchemaDrivenAnnotationEditor/template/ButtonTemplate.tsx

+  )
+}
+
+export default {


Ability to simply override buttons didn't exist before RJSF v5.

nickgros · 2022-10-13T13:11:00Z

src/lib/containers/entity/SchemaDrivenAnnotationEditor/template/DescriptionFieldTemplate.tsx

+import React from 'react'
+import Typography from '../../../../utils/typography/Typography'
+
+export default function DescriptionFieldTemplate(props: DescriptionFieldProps) {


Used to be FieldDescriptionTable.tsx and was manually inserted into each template. Now, we can just override the DescriptionFieldTemplate.

nickgros · 2022-10-13T13:11:39Z

src/lib/containers/entity/SchemaDrivenAnnotationEditor/template/FieldTemplate.tsx

+import { HelpOutline } from '@material-ui/icons'
+import { Collapse } from '@material-ui/core'
+
+export function FieldTemplate<T>(props: FieldTemplateProps<T>) {


Used to be CustomDefaultTemplate. Now is just concerned with layout

nickgros · 2022-10-13T13:13:27Z

src/lib/containers/entity/SchemaDrivenAnnotationEditor/template/ObjectFieldTemplate.tsx

@@ -13,10 +20,35 @@ import { displayToast } from '../../ToastMessage'
 * @param props
 * @returns
 */
-export function CustomObjectFieldTemplate(
-  props: ObjectFieldTemplateProps<Record<string, unknown>>,
+export function ObjectFieldTemplate<T, F>(


Mostly code style changes in this component to match the base RJSF v5 ObjectFieldTemplate

nickgros · 2022-10-13T13:16:11Z

src/lib/containers/entity/SchemaDrivenAnnotationEditor/template/WrapIfAdditionalTemplate.tsx

-    onKeyChange: (newKey: string) => void
-  },
-) => {
+export default function WrapIfAdditionalTemplate<T = any, F = any>(


For additionalProperties, we were overriding an entire FieldTemplate to alter the layout of an internal RJSF component called WrapIfAdditional. We added WrapIfAdditionalTemplate to RJSF v5 to make this easier.

nickgros force-pushed the rjsf-v5 branch from f03ca61 to ec0a127 Compare October 11, 2022 16:25

nickgros added 2 commits October 11, 2022 12:32

Upgrade to rjsf-v5

5882268

Tests still failing.

Upgrade to rjsf-v5

4408aa9

Changes to fix tests and build errors

nickgros force-pushed the rjsf-v5 branch from ec0a127 to 4408aa9 Compare October 12, 2022 18:12

nickgros marked this pull request as ready for review October 12, 2022 18:13

nickgros commented Oct 12, 2022

View reviewed changes

Upgrade to rjsf-v5

4d2d1d3

Fix test suite broken from moving files

nickgros commented Oct 12, 2022

View reviewed changes

nickgros commented Oct 13, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to RJSF v5 #1819

Upgrade to RJSF v5 #1819

nickgros commented Oct 5, 2022 •

edited

Loading

nickgros Oct 12, 2022

nickgros Oct 12, 2022 •

edited

Loading

nickgros Oct 12, 2022 •

edited

Loading

nickgros Oct 12, 2022

nickgros Oct 12, 2022

nickgros Oct 12, 2022

nickgros Oct 12, 2022

nickgros Oct 12, 2022

nickgros Oct 12, 2022

nickgros Oct 12, 2022

nickgros Oct 13, 2022 •

edited

Loading

nickgros Oct 13, 2022

nickgros Oct 13, 2022

nickgros Oct 13, 2022

nickgros Oct 13, 2022

nickgros Oct 13, 2022

nickgros Oct 13, 2022

nickgros Oct 13, 2022

nickgros Oct 13, 2022

		// TODO: Test is not working, but the functionality is working in the browser
		it.skip('Removes a custom annotation field when the last value is removed', async () => {

Upgrade to RJSF v5 #1819

Are you sure you want to change the base?

Upgrade to RJSF v5 #1819

Conversation

nickgros commented Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

nickgros Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

nickgros Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickgros Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickgros commented Oct 5, 2022 •

edited

Loading

nickgros Oct 12, 2022 •

edited

Loading

nickgros Oct 12, 2022 •

edited

Loading

nickgros Oct 13, 2022 •

edited

Loading