Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in nested ArrayInput with initialValue(s) #6908

Closed
wants to merge 4 commits into from

Conversation

DjebbZ
Copy link
Contributor

@DjebbZ DjebbZ commented Nov 26, 2021

What you were expecting:
When I open the Edit view of a resource that has nested ArrayInput and use some initialValue(s) both at the form level and the field, the inputs for this part of the data shows the value from the dataProvider.

What happened instead:
Instead, the inputs display the initialValue at the field level, making it easy for editor that modify another part of the record to override the existing data!

Steps to reproduce:

  1. Open the code sandbox https://codesandbox.io/s/reproduce-nested-arrayinput-default-value-bfpnr
  2. Open the Edit view of the post "Accusantium2 qui nihil voluptatum" (its url in the internal browser is https://bfpnr.sse.codesandbox.io/#/posts/1/2)
  3. Look at the "Related Pictures" inputs : they're all empty because the inputs have initialValue set to "" but data.tsx (should be data.ts BTW) has actual data.

Related code:
The sandbox https://codesandbox.io/s/reproduce-nested-arrayinput-default-value-bfpnr has all the code, and I created this pull request to highlight the small set of changes I made to reproduce the bug.

Other information:
It may be related to a similar issue we created related to <BooleanIinput /> in nested ArrayInput, not sure (#6511)

Environment

  • React-admin version: 3.19.0, 3.19.2
  • Last version that did not exhibit the issue (if applicable): N/A, we're not sure when the problem manifested itself.
  • React version: 17
  • Browser: Chrome, Brave, Firefox
  • Stack trace (in case of a JS error):

@DjebbZ DjebbZ changed the title Reproduce bug in nested ArrayInput with initialValue(s) Bug in nested ArrayInput with initialValue(s) Nov 26, 2021
@WiXSL
Copy link
Contributor

WiXSL commented Nov 26, 2021

Could you describe the bug you are trying to fix?

@DjebbZ
Copy link
Contributor Author

DjebbZ commented Nov 26, 2021

Done, I updated the description (I created the PR from CodeSandbox without a description nor a proper title)

@DjebbZ
Copy link
Contributor Author

DjebbZ commented Nov 26, 2021

(Sorry if I created a PR instead of an issue, but the diff directly in GitHub looked like a good idea to me)

@DjebbZ
Copy link
Contributor Author

DjebbZ commented Nov 29, 2021

It may be related to this issue #6900, not sure though.

@DjebbZ
Copy link
Contributor Author

DjebbZ commented Nov 29, 2021

I tracked it down to useInput inside of RA's <TextField />, that return a value of "" (which is the initialValue, not the actual value from the record) and passes it to the ResettableTextField underlying component. It gets this value from useInput, that itself uses useField from react-final-form. Maybe the problem is over there ?

@djhi
Copy link
Collaborator

djhi commented Nov 29, 2021

Hi @DjebbZ,
Unfortunately, your code formater settings make it very difficult to see what you actually changed. Can you revert the unnecessary changes?

Comment on lines 91 to 100
<ArrayField source="pictures">
<Datagrid>
<ImageField source="url" />
<ArrayField source="metas.authors">
<Datagrid>
<TextField source="name" />
</Datagrid>
</ArrayField>
</Datagrid>
</ArrayField>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I added to the simple example (+ updating react-admin in package.json)

@DjebbZ
Copy link
Contributor Author

DjebbZ commented Nov 29, 2021

Hi @DjebbZ, Unfortunately, your code formater settings make it very difficult to see what you actually changed. Can you revert the unnecessary changes?

Replied via a comment in the diff, because it's too tedious to revert just the code styles. No idea why your prettier config wasn't caught by my IDE.

Comment on lines 209 to 218
<ArrayInput source="pictures">
<SimpleFormIterator>
<TextInput source="url" initialValue="" />
<ArrayInput source="metas.authors">
<SimpleFormIterator>
<TextInput source="name" initialValue="" />
</SimpleFormIterator>
</ArrayInput>
</SimpleFormIterator>
</ArrayInput>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I also added this part (this is in fact the most important part of the bug report)

@djhi
Copy link
Collaborator

djhi commented Nov 29, 2021

Got it, reproduced

@djhi djhi added the bug label Nov 29, 2021
return (
<Edit title={<PostTitle />} actions={<EditActions />} {...props}>
<TabbedForm
initialValues={_.merge({}, { average_note: 0, pictures: [] })}
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 forgot to say that I also I added this part : a global object for the initialValues. Since the ArrayInput field prevents us from specifying here the shape of the pictures field, I had to also add initialValue at the field level. This global initialValues may be important for the bug.

@djhi
Copy link
Collaborator

djhi commented Nov 29, 2021

So, I tried to reproduce it locally both on master and the v3.19.2 tag branch but couldn't. I can only reproduce it on codesandbox

@DjebbZ
Copy link
Contributor Author

DjebbZ commented Nov 29, 2021

Have you looked at my last comment ? (initialValues at the form level). We commented at ~the same time, maybe you missed it when trying to reproduce ?

@djhi
Copy link
Collaborator

djhi commented Nov 29, 2021

I did and tried in both branches

@DjebbZ
Copy link
Contributor Author

DjebbZ commented Nov 30, 2021

Me neither, I'm trying to figure out what's missing and will update the PR.

@DjebbZ
Copy link
Contributor Author

DjebbZ commented Nov 30, 2021

I managed to reproduce, that's quite a sneaky one.

In CodeSandbox, according to the docs, it will run the dev script so the simple example is run through webpack. You can see it clearly in the Terminal output in CodeSandbox $ ./node_modules/.bin/webpack-dev-server --progress --color --hot --watch --mode development (which corresponds to the dev script in the simple example's package.json).

But when trying to reproduce locally, I use (and I'm you did too) the make run-simple command, which runs the start command of the simple example, that use vite, not webpack.

And in our application, we use create-react-app which uses webpack internally.

If you run the simple example locally with yarn run dev inside examples/simple, you'll see the problem.

@DjebbZ
Copy link
Contributor Author

DjebbZ commented Nov 30, 2021

I also updated the PR code with my local reproduction case, now the diff is very clean and easy to read.

@DjebbZ
Copy link
Contributor Author

DjebbZ commented Nov 30, 2021

Regarding Vite vs Webpack: that might mean that the way Webpack transpiles the code produces the bug, whereas with Vite it doesn't happen. I've also noticed after a quick glance that the vite.config.js does a few things that webpack.config.js doesn't do, i.e. pointing react-admin to the local version, not the one stored in node_modules. Maybe the bug comes from those kind of differences in the build config?

@djhi
Copy link
Collaborator

djhi commented Nov 30, 2021

Thanks for the details! I'll dig

@DjebbZ
Copy link
Contributor Author

DjebbZ commented Dec 2, 2021

Any news ? Any way I can help investigate the issue ? Any pointer welcome to help you debug this, I don't really know where to start.

@djhi
Copy link
Collaborator

djhi commented Dec 2, 2021

I can now reproduce it locally on master even with vite if I remove the aliases.

@djhi
Copy link
Collaborator

djhi commented Dec 2, 2021

Found it! It looks like a regression in final-form. I still have to find the exact source of this issue though.
Workaround for now, install final-form@4.20.2 in your project

@DjebbZ
Copy link
Contributor Author

DjebbZ commented Dec 2, 2021

I can confirm that the workaround works, thanks A LOT!

Are you capable of telling since which version of RA this regression caused the bug? I want to estimate the potential damage in our app.

@djhi
Copy link
Collaborator

djhi commented Dec 3, 2021

Actually, it shouldn't have worked before. Final-form array fields are dynamic by nature and the documentation clearly state that the initialValue specified on a field overrides the form initial value. We use this form initial value to initialize the form with the current record. I have a fix that will be published in 3.19.3.

Are you capable of telling since which version of RA this regression caused the bug? I want to estimate the potential damage in our app

Nope

@DjebbZ
Copy link
Contributor Author

DjebbZ commented Dec 3, 2021

Thanks for detailed answer.

@DjebbZ
Copy link
Contributor Author

DjebbZ commented Dec 7, 2021 via email

@DjebbZ
Copy link
Contributor Author

DjebbZ commented Dec 7, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants