Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

Fix Formstate.Nested in Formstate.List #698

Merged
merged 1 commit into from
May 15, 2019
Merged

Conversation

axelerator
Copy link
Contributor

@axelerator axelerator commented May 13, 2019

Using a nested property in a list would result in a "Cannot read property
'brand' of undefined" error when adding new items to the list.

The error looks something like this:

Cannot read property 'brand' of undefined
TypeError: 
    at eval (webpack:///./node_modules/@shopify/react-form-state/dist/components/Nested.js?:33:49)
    at eval (webpack:///./node_modules/@shopify/react-form-state/dist/utilities.js?:9:28)
    at Array.reduce (<anonymous>)
    at Object.mapObject (webpack:///./node_modules/@shopify/react-form-state/dist/utilities.js?:7:34)
    at Nested.render (webpack:///./node_modules/@shopify/react-form-state/dist/components/Nested.js?:32:39)

This may also address #571

Minimal broken example of broken

function App() {
  return (
    <AppProvider>
    <div className="App">
      <header className="App-header">
        <FormState
        initialValues={{
          variants: [
            {
              description: {
                brand: 'brandName1',
              },
            },
          ],
        }}
      >
        {({fields}) => {
          const {variants} = fields;
          function addVariant() {
            variants.onChange(
              arrayUtils.push(variants.value, {
                description: {
                  brand: 'brandName3',
                },
              }),
            );
          }
          return (
            <form>
              <div>
                  <div>
                    <FormState.List field={fields.variants}>
                      {(fields) => {
                        return (
                          <FormState.Nested field={fields.description}>
                            {(nestedFields) => {
                              return (
                                <TextField
                                  label=""
                                  {...nestedFields.brand}
                                />
                              );
                            }}
                          </FormState.Nested>
                        );
                      }}
                    </FormState.List>

                    <ButtonGroup>
                      <Button onClick={addVariant}>Add variant</Button>
                    </ButtonGroup>
                  </div>
              </div>
            </form>
          );
        }}
      </FormState>
        <p>
          Edit <code>src/App.js</code> and save to reload.
        </p>
        <a
          className="App-link"
          href="https://reactjs.org"
          target="_blank"
          rel="noopener noreferrer"
        >
          Learn React
        </a>
      </header>
    </div>
    </AppProvider>
  );
}

@axelerator axelerator force-pushed the formstate-list-nested-fix branch from 2131ed3 to 25aa97d Compare May 13, 2019 14:33
@axelerator axelerator requested review from marutypes and a team May 13, 2019 18:06
Copy link
Member

@lemonmade lemonmade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I need a bit of an explanation as to how this is hit, and I'd like @TheMallen to review.

let initialFieldValue: any;
if (initialValue) {
initialFieldValue = initialValue[fieldPath];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would probably be a bit cleaner to do const initialFieldValue = initialValue && initialValue[fieldPath]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit confused how this could ever happen if the field type is not an optional type; that is, in the example case you provided, why is initialValue undefined? It looks like initialValue should be {brand: 'brandName1'}, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure - but I think it's du to the fact that an added list item is not present in the initial data passed to the FormState.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the example you provided (very helpful btw), the initial values are definitely present. I wonder if this is really more of a bug in List not passing down the value to the render prop correctly? Can you log out what fields.description is?

Copy link
Contributor Author

@axelerator axelerator May 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error happens only when you add a new element with the Add variant button via arrayUtils to the list - this new, second item is not present in the data initially passed to <FormState initialValues=.., which has only one element in the variants list

Copy link
Contributor

@marutypes marutypes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR :)

Code change makes sense (though I agree with chris's point about clarity), can we add a test to make sure this doesn't regress and update the CHANGELOG.md before this ships?

@axelerator axelerator force-pushed the formstate-list-nested-fix branch from 25aa97d to e883b36 Compare May 15, 2019 19:25
@axelerator axelerator requested a review from marina101 May 15, 2019 19:27
@axelerator axelerator force-pushed the formstate-list-nested-fix branch from e883b36 to 9f2a380 Compare May 15, 2019 19:28
Using a nested property in a list would result in a "Cannot read property
'brand' of undefined" error when adding new items to the list.
@axelerator axelerator force-pushed the formstate-list-nested-fix branch from 9f2a380 to 9e53b5c Compare May 15, 2019 19:39
Copy link
Contributor

@marina101 marina101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@axelerator
Copy link
Contributor Author

I added a test and a changelog entry

@axelerator axelerator merged commit 6abd166 into master May 15, 2019
@michenly michenly temporarily deployed to production May 15, 2019 21:19 Inactive
@BPScott BPScott deleted the formstate-list-nested-fix branch May 22, 2021 00:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants