Skip to content

Commit 5e433aa

Browse files
authored
perf(ui): reduce number of field renders on submit (#13524)
Needed for #13501. No need to re-render all fields during save, and especially autosave. Fields are already rendered. We only need to render new fields that may have been created by hooks, etc. We can achieve this by sending previous form state and new data through the request with `renderAllFields: false`. That way form state can be built up from previous form state, while still accepting new data. --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1211094406108904
1 parent c7b9f0f commit 5e433aa

File tree

5 files changed

+36
-26
lines changed

5 files changed

+36
-26
lines changed

packages/ui/src/forms/Form/index.tsx

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -266,12 +266,12 @@ export const Form: React.FC<FormProps> = (props) => {
266266

267267
const data = reduceFieldsToValues(contextRef.current.fields, true)
268268

269+
const serializableFormState = deepCopyObjectSimpleWithoutReactComponents(
270+
contextRef.current.fields,
271+
)
272+
269273
// Execute server side validations
270274
if (Array.isArray(beforeSubmit)) {
271-
const serializableFormState = deepCopyObjectSimpleWithoutReactComponents(
272-
contextRef.current.fields,
273-
)
274-
275275
let revalidatedFormState: FormState
276276

277277
await beforeSubmit.reduce(async (priorOnChange, beforeSubmitFn) => {
@@ -379,6 +379,7 @@ export const Form: React.FC<FormProps> = (props) => {
379379
if (typeof onSuccess === 'function') {
380380
const newFormState = await onSuccess(json, {
381381
context,
382+
formState: serializableFormState,
382383
})
383384

384385
if (newFormState) {

packages/ui/src/forms/Form/mergeServerFormState.ts

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,10 @@ type Args = {
3030
* We typically do not want to merge properties that rely on user input, however, such as values, unless explicitly requested.
3131
* Doing this would cause the client to lose any local changes to those fields.
3232
*
33-
* This function will also a few defaults, as well as clean up the server response in preparation for the client.
33+
* Note: Local state is the source of truth, not the new server state that is getting merged in. This is critical for array row
34+
* manipulation specifically, where the user may have added, removed, or reordered rows while a request was pending and is now stale.
35+
*
36+
* This function applies some defaults, as well as cleans up the server response in preparation for the client.
3437
* e.g. it will set `valid` and `passesCondition` to true if undefined, and remove `addedByServer` from the response.
3538
*/
3639
export const mergeServerFormState = ({
@@ -51,22 +54,30 @@ export const mergeServerFormState = ({
5154
* a. accept all values when explicitly requested, e.g. on submit
5255
* b. only accept values for unmodified fields, e.g. on autosave
5356
*/
54-
if (
55-
!incomingField.addedByServer &&
56-
(!acceptValues ||
57-
// See `acceptValues` type definition for more details
58-
(typeof acceptValues === 'object' &&
59-
acceptValues !== null &&
60-
acceptValues?.overrideLocalChanges === false &&
61-
currentState[path].isModified))
62-
) {
63-
delete incomingField.value
64-
delete incomingField.initialValue
57+
const shouldAcceptValue =
58+
incomingField.addedByServer ||
59+
acceptValues === true ||
60+
(typeof acceptValues === 'object' &&
61+
acceptValues !== null &&
62+
// Note: Must be explicitly `false`, allow `null` or `undefined` to mean true
63+
acceptValues.overrideLocalChanges === false &&
64+
!currentState[path]?.isModified)
65+
66+
let sanitizedIncomingField = incomingField
67+
68+
if (!shouldAcceptValue) {
69+
/**
70+
* Note: do not delete properties off `incomingField` as this will mutate the original object
71+
* Instead, omit them from the destructured object by excluding specific keys
72+
* This will also ensure we don't set `undefined` into the result unnecessarily
73+
*/
74+
const { initialValue, value, ...rest } = incomingField
75+
sanitizedIncomingField = rest
6576
}
6677

6778
newState[path] = {
6879
...currentState[path],
69-
...incomingField,
80+
...sanitizedIncomingField,
7081
}
7182

7283
if (

packages/ui/src/forms/Form/types.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ export type FormProps = {
6161
* Arbitrary context passed to the onSuccess callback.
6262
*/
6363
context?: Record<string, unknown>
64+
formState?: FormState
6465
},
6566
) => Promise<FormState | void> | void
6667
redirect?: string

packages/ui/src/utilities/buildFormState.ts

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -134,8 +134,6 @@ export const buildFormState = async (
134134

135135
const selectMode = select ? getSelectMode(select) : undefined
136136

137-
let data = incomingData
138-
139137
if (!collectionSlug && !globalSlug) {
140138
throw new Error('Either collectionSlug or globalSlug must be provided')
141139
}
@@ -175,11 +173,9 @@ export const buildFormState = async (
175173
)
176174
}
177175

178-
// If there is a form state,
179-
// then we can deduce data from that form state
180-
if (formState) {
181-
data = reduceFieldsToValues(formState, true)
182-
}
176+
// If there is form state but no data, deduce data from that form state, e.g. on initial load
177+
// Otherwise, use the incoming data as the source of truth, e.g. on subsequent saves
178+
const data = incomingData || reduceFieldsToValues(formState, true)
183179

184180
let documentData = undefined
185181

packages/ui/src/views/Edit/index.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ export function DefaultEditView({
258258

259259
const onSave = useCallback<FormProps['onSuccess']>(
260260
async (json, options) => {
261-
const { context } = options || {}
261+
const { context, formState } = options || {}
262262

263263
const controller = handleAbortRef(abortOnSaveRef)
264264

@@ -327,9 +327,10 @@ export function DefaultEditView({
327327
data: document,
328328
docPermissions,
329329
docPreferences,
330+
formState,
330331
globalSlug,
331332
operation,
332-
renderAllFields: true,
333+
renderAllFields: false,
333334
returnLockStatus: false,
334335
schemaPath: schemaPathSegments.join('.'),
335336
signal: controller.signal,

0 commit comments

Comments
 (0)