Can't use ModernTaxonomyPicker in a controlled way

[stevebeauge]

Category

[ ] Enhancement

[x] Bug

[x] Question

Version

Please specify what version of the library you are using: [ 3.10.0 ] (worked fine with 3.9.0)

Expected / Desired Behavior / Question

It's a common pattern to "control" the value of fields by using some kind of state and define values from the state, then update the state from a onchange

The fluent UI lib for example is full of such pattern (https://developer.microsoft.com/en-us/fluentui#/controls/web/peoplepicker#implementation)

It should possible to reproduce same pattern with ModernTaxonomyPicker.

Observed Behavior

If setting initialValues from something that came from the state, the control goes into a infinite loop (which ends in being throttled)

If setting initialValues from a prop, whenever the control change, it resets to the initial values.

Steps to Reproduce

Repro : https://github.com/stevebeauge/pnpreactrepromoderntaxonomy/blob/51969c31f270b51ac5ea4332e232d6a7aa27aa1d/src/webparts/repro/components/Repro.tsx#L1-L53

import * as React from 'react';


import { IModernTaxonomyPickerProps, ModernTaxonomyPicker } from '@pnp/spfx-controls-react';

import { BaseComponentContext } from '@microsoft/sp-component-base';
import { TextField } from 'office-ui-fabric-react';
import { useState } from 'react';

type ReproProps = {
  context: BaseComponentContext
};

type PartialTermInfo = IModernTaxonomyPickerProps["initialValues"];

const Repro = ({ context }: ReproProps): React.ReactElement<ReproProps> => {

  const [termSetId, setTermSetId] = useState<string | undefined>(undefined);
  const [selectedTerms, setSelectedTerms] = useState<PartialTermInfo | undefined>(undefined);

  return (
    <>
      <div>
        <TextField value={termSetId} onChange={(evt, newValue) => setTermSetId(newValue)} />
      </div>
      <div>
        {termSetId && (
          <ModernTaxonomyPicker
            termSetId={termSetId}
            panelTitle="My term set"
            label="Modern taxonomy picker"
            context={context}
            initialValues={selectedTerms}
            onChange={setSelectedTerms}
          />
        )}
      </div>
      <div>
        {selectedTerms && (
          <ul>
            {selectedTerms.map(t => (
              <li key={t.id}>{t.labels[0].name}</li>
            ))
            }
          </ul>
        )
        }
      </div>
    </>
  );
}

export { Repro, ReproProps };

[ghost]

Thank you for reporting this issue. We will be triaging your incoming issue as soon as possible.

[omarelanis]

@stevebeauge you're code is incorrect which is why you are experiencing this issue, the initialValues is only used to set what is initially selected when you first load the component and should not be updated post component render as it will re-render the whole control.

Imagine for example you have a taxonomy field which already has "Marketing", when the control renders you will send through the object that contains "Marketing" so the control will render it as already having been selected. If you try and manipulate the state that initialValues is using after component render it will break as you are seeing there.

Instead use the onChange property to send any updates to the selection to a state property you are controlling separately, that event will include the initialValues value if they remained selected in the control, if you have since removed one of the taxonomy choices they will not be sent to the onChange event.

for example:
`
// Get the term ID of the site field to be used for the control
const termID = await sp.web.fields.getByInternalNameOrTitle("MyTaxonomyField").select("Id", "TermSetId").get();

// Get any pre-selected choices for a specific taxonomy field
const taxInitValues: ITermInfo = await sp.termStore.groups.getById("GROUP ID").sets.getById("TERM ID").getTermById("THIS IS THE GUID OF THE TAXONOMY FIELD IN THE SHAREPOINT ITEM")();

< ModernTaxonomyPicker
allowMultipleSelections={false}
label="Operation Unit Type"
termSetId= termID
panelTitle="Select Operation Unit Type"
context={context}
onChange={}
initialValues={taxInitValues}
disabled={this.state.stage1LockedFields}
/>
`
The above code will load the data including any pre-selected taxonomy choices from the field and display it in the control, next you can add in an onChange function to store the changes and then submit these back to SharePoint as needed.

Hope that makes sense?

[stevebeauge]

Sorry @omarelanis, but I still think there's something missing. What you show here will not allow setting the selected terms outside the component itself.

What I tried also :

type PartialTermInfo = IModernTaxonomyPickerProps["initialValues"];

const Repro = ({ context }: ReproProps): React.ReactElement<ReproProps> => {

  const [termSetId, setTermSetId] = useState<string | undefined>(undefined);
  const initialTerms: PartialTermInfo = [
    {
      id: "5abaeda2-1685-431c-af4e-45e76310754c",
      labels: [
        {
          isDefault: true,
          languageTag: 'fr-FR',
          name: "Marketing"
        }
      ]
    }
  ];
  const [selectedTerms, setSelectedTerms] = useState<PartialTermInfo | undefined>(undefined);

  return (
    <>
      <div>
        <TextField value={termSetId} onChange={(evt, newValue) => setTermSetId(newValue)} />
      </div>
      <div>
        {termSetId && (
          <ModernTaxonomyPicker
            allowMultipleSelections
            termSetId={termSetId}
            panelTitle="My term set"
            label="Modern taxonmy picker"
            context={context}
            initialValues={initialTerms}
            onChange={setSelectedTerms}
          />
        )}
      </div>
.....
....

This populates the control with default value, but the field is like "readonly". Because onChange triggers the rerender of the control, the initial values are set again and the field keeps the only selection

To clarify, imagine the ModernTaxonomyPicker is part of a large forms, when field values can be set according some rules. The values are stored in a outer state and may be changed by something else. There's nothing that can set value of the control, but the initial values.

For example, the react hook form can embed 3rd party library to build complex forms. There's some callback method that provide a value and a onchange method to populate the field.

Specifically:

import { IModernTaxonomyPickerProps, ModernTaxonomyPicker } from '@pnp/spfx-controls-react';
import React from 'react';
import { Controller, Control, ControllerProps, Path } from 'react-hook-form';

type ControlledType<F extends object, T> = {
    name: Path<F>;
    control: Control<F>;
    rules?: ControllerProps['rules'];
} & T;

type ControlledTaxonomyPickerProps<F extends object> = ControlledType<
    F,
    Omit<IModernTaxonomyPickerProps, 'onChange' | 'initialValues'>
>;

const ControlledTaxonomyPicker = <F extends object>({
    name,
    control,
    ...props
}: ControlledTaxonomyPickerProps<F>): JSX.Element => {
    return (
        <Controller
            name={name}
            control={control}
            render={({ field: { onChange, value } }) => {
                return <ModernTaxonomyPicker initialValues={value} onChange={onChange} {...props} />;
            }}
        />
    );
};

export { ControlledTaxonomyPicker };

This snippet is how we use the control in a "controlled" way. W/ initial values, the control loops forever. W/o, the control's content is reset each time it's rendered again.

Under the hood I guess the root cause is that the type of the component value is an array. Everytime the component emit a onChange event, a new instance of the array is throw, even if the values are the same, and the react engine consider it has to rerender.

[omarelanis]

Hi @stevebeauge, I've attached a sample gif of how the control should work when you have it setup correctly:

With the first code snippet can you change allowMultipleSelections to allowMultipleSelections={false}
Also does that first code snippet still load the element as read only so you can't change the value?

The key thing here is the control manages the state of the selected items itself within the component, you can extract the values with the onChange function but you should not be trying to update the component using the state

When I get some time I'll try and setup your repo and see if I can replicate the issue

[stevebeauge]

I've extended my reproduction repository with a new webpart that reflect more the actual implementation:

https://github.com/stevebeauge/pnpreactrepromoderntaxonomy/blob/92152e2a8ed1112efada88d22d9a5852ee75bf8c/src/webparts/reproInForm/components/ReproInForm.tsx#L111-L137

This contains a workaround I found to avoid the behavior described in the issue.

The trick consists in intercepting the onChange event and only bubble this event if the selection actually changed. I compare the selected IDs to perform this check.

        <Controller
          name="someTaxoVal"
          control={control}
          render={({ field: { onChange, value, } }) => {
            const termSetId = getValues('termSetId');
            return (
              termSetId ? (
                <ModernTaxonomyPicker
                  termSetId={termSetId}
                  panelTitle={'Choose a term'}
                  label={''}
                  allowMultipleSelections
                  onChange={(selection) => {
                    // Actual output of onChange is not serializable, so wrap it in minimal required value
                    const newVal = selection.map<TermId>(term => ({ id: term.id, label: term.labels[0].name, languageTag: term.labels[0].languageTag }));
                    // Check if term ids have actually changed by comparing IDs
                    const selectedKeys = value.map(s => s.id);
                    const newKeys = selection.map(s => s.id);
                    const isSame = (
                      selectedKeys.length === newKeys.length &&
                      selectedKeys.every(id => newKeys.indexOf(id) !== -1)
                    );
                    if (!isSame) {
                      onChange(newVal);
                    }
                  }}
                  initialValues={
                    value.map(v => ({
                      id: v.id,
                      labels: [{ name: v.label, isDefault: true, languageTag: v.languageTag }]
                    }))
                  }
                  context={context}
                />) : (
                <Shimmer />
              ));
          }
          } />

This way, the inifinite loop does not occurs

[AJIXuMuK]

Infinite loop may appear only if initialValues (or any other property from the dependency) is changed.
If the values are the same useEffect will not be called.

The change from the PR is actually removing the "controlled" way completely. Changes to initialValues will never be reflected.

[stevebeauge]

Infinite loop may appear only if initialValues (or any other property from the dependency) is changed. If the values are the same useEffect will not be called.

The change from the PR is actually removing the "controlled" way completely. Changes to initialValues will never be reflected.

@AJIXuMuK probably the way I fix the issue in the PR is not the good way to solve it. That said, starting from the 3.10, the infinite loop appears (see my repro).

The use of useState should cause actually two render. The first with the initial values partially resolved, then the second with the fully resolved picker values. But this second iteration also cause the onchange callback even if the values are (logicially) the same.

One of my hypothesis is that the array of object nature of the value make react consider it's a new value, even if individual properties are the same.

Other hypothesis is that value is not serializable, because of the Parent attribute. It creates a possible circular reference in the value walking.

[stevebeauge]

If it can help, here's how I overcome the bug, without the PR. As I suspected, moving the parent attribute of the state seems to solve the issue:

import { IModernTaxonomyPickerProps, ModernTaxonomyPicker } from '@pnp/spfx-controls-react/lib/ModernTaxonomyPicker';
import React from 'react';

// Workaroung bug https://github.com/pnp/sp-dev-fx-controls-react/issues/1306
// We want to remove the "parent" attribute of the term to remove the infinite loop

// Extract original type
type OriginalTermInfoValue = IModernTaxonomyPickerProps['initialValues'];

// remove the parent attribute
export type ITermInfoNoParent = Omit<NonNullable<OriginalTermInfoValue>[0], 'parent'>;
export type ITermInfoNoParentArray = ITermInfoNoParent[] | undefined;

// Tweak the taxonomy picker props
interface IWrappedModernTaxonomyPickerProps extends Omit<IModernTaxonomyPickerProps, 'onChange' | 'initialValues'> {
    //onChangeTaxonomyHandler: (taxonomyArray: any) => void;
    initialValues?: ITermInfoNoParentArray;
    onChange?: (newValue: ITermInfoNoParentArray) => void;
}

const WrappedModernTaxonomyPicker = (props: IWrappedModernTaxonomyPickerProps) => {
    const pnpInitialValues: OriginalTermInfoValue = props.initialValues?.map((ti) => ({
        ...ti,
    }));
    return (
        <>
            <ModernTaxonomyPicker
                {...props}
                onChange={(selection) => {
                    if (props.onChange) {
                        const newVal = selection?.map((sel) => {
                            const { parent, ...others } = sel; // Remove the parent attribute
                            return { ...others };
                        });
                        props.onChange(newVal);
                    }              
                }}
                initialValues={pnpInitialValues}
            />
        </>
    );
};

export { WrappedModernTaxonomyPicker };

This way I can switch the pnp's ModernTaxonomyPicker by WrappedModernTaxonomyPicker to make it works. It'll work until I need to play with parent

[AJIXuMuK]

@patrikhellgren has fixed the infinite loop and also stated (as the owner of the control) that it CANNOT be used in the controlled way without a refactoring.
See the linked PR.