VulcanJS / Vulcan

🌋 A toolkit to quickly build apps with React, GraphQL & Meteor
http://vulcanjs.org
MIT License
7.98k stars 1.89k forks source link

Nested Schema document update not working #2686

Closed EloyID closed 3 years ago

EloyID commented 3 years ago

Describe the bug If I have a Nested Schema using the MuiSelect in a SmartForm it will not change the shown value since the document prop passed to the input does not update.

To Reproduce Steps to reproduce the behavior:

  1. Use MaterialUI package
  2. Create a nested schema with a input 'select'
  3. Try to update the value

Expected behavior The value to change

Screenshots (if applicable) If applicable, add screenshots to help explain your problem.

Additional context In the back end it works : if I save the form the changes are applied even if the value in the front has not changed. If I use the MuiInput (text) there is no problem because it updates the value inside (as if it was not controlled) until it's updated from outside (what happens normally but not in nested schemas) https://github.com/VulcanJS/Vulcan/blob/0c15ba74cf8cd1ca90021c9bdf6c3e252b39f625/packages/vulcan-ui-material/lib/components/forms/base-controls/FormInput.jsx#L119 while in MuiSelect it always depens on the passed value https://github.com/VulcanJS/Vulcan/blob/0c15ba74cf8cd1ca90021c9bdf6c3e252b39f625/packages/vulcan-ui-material/lib/components/forms/base-controls/FormSelect.jsx#L158

I have followed the error and it comes from here https://github.com/VulcanJS/Vulcan/blob/0c15ba74cf8cd1ca90021c9bdf6c3e252b39f625/packages/vulcan-forms/lib/components/FormNestedItem.jsx#L41 There the props have the right updated document but it's overrided by the ...field which has a wrong document

If I'm not wrong it comes from https://github.com/VulcanJS/Vulcan/blob/0c15ba74cf8cd1ca90021c9bdf6c3e252b39f625/packages/vulcan-forms/lib/components/Form.jsx#L472

ErikDakoda commented 3 years ago

Hi @EloyID! I dug into this and could confirm this bug. I created a PR to fix it - along with a couple of small Form-related issues.

At first I fixed the issue by switching the order of {...props} and {...field} in FormNestedItem:

https://github.com/VulcanJS/Vulcan/blob/0c15ba74cf8cd1ca90021c9bdf6c3e252b39f625/packages/vulcan-forms/lib/components/FormNestedItem.jsx#L39

          <FormComponents.FormComponent
            key={i}
            {...field}
            {...props}
            path={`${path}.${field.name}`}
            itemIndex={itemIndex}
          />

But I was worried that this may have unintended side effects. Then I decided to just override the document props:

          <FormComponents.FormComponent
            key={i}
            {...props}
            {...field}
            document={props.document}
            path={`${path}.${field.name}`}
            itemIndex={itemIndex}
          />

That works, but I was worried that it does not address the source of the problem, which is, as you already guessed, in Form.jsx. So the PR fixes the problem in SmartForm.initField(), but I want somebody who is more familiar with this code to have a look before I merge the PR.

EloyID commented 3 years ago

Hi I have tried to go deeper in the code to be sure but it will be better if somebody more familiar checks it. I have had a look in the PRs and even if it fixes it for now, I can't understand why it works for normal fields but not for the nested ones. Both fields pass through Form.jsx > createField. So maybe we are having two source of truth what can be a cause of other problems later.

Following the same reasoning, MuiInput worked in the nested schema but no the MuiSelect, because the first one is not really controlled (it updates its value before the parent tells) while MuiSelect didn't. For me the MuiSelect is a better approach because in the future we can have bugs because we think the value is updating correctly in the back when it's only doing in the front.