Skip to content

move and swap break form inputs #15

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

Closed
carlhueffmeier opened this issue Sep 9, 2018 · 12 comments
Closed

move and swap break form inputs #15

carlhueffmeier opened this issue Sep 9, 2018 · 12 comments

Comments

@carlhueffmeier
Copy link
Contributor

Are you submitting a bug report or a feature request?

I believe I found a bug.

What is the current behavior?

These are the steps to reproduce:

  1. Create two input fields
  2. Swap them
  3. Type into the first input field
  4. The typed character will not appear in the first input field, but in the second

What is the expected behavior?

Swapping or moving fields should preserve behavior.

Sandbox Link

Edit 🏁 React Final Form - Field Arrays

To keep things as simple as possible, I reused the sandbox from the issue referenced below, simply upgrading the library version to 1.1.0.

What's your environment?

The problem occurs with final-form-arrays version 1.1.0. Downgrading the package to 1.0.6 solves the problem.

Other information

I believe the problem is caused by the latest change to the move and swap code in response to this previous issue.

This commit looks especially suspicious.

I would be happy to work on it, if you give me some pointers on where to start 💪

@maciejmyslinski
Copy link

Yeah, I believe you've found a bug, indeed. I'll write a test case for it.

@carlhueffmeier
Copy link
Contributor Author

That would be great! I could look into it this weekend.

@tlenclos
Copy link

tlenclos commented Nov 2, 2018

Hey guys, any news on this or some lead ? I can't implement a drag&drop sorting sadly due to this bug.

@carlhueffmeier
Copy link
Contributor Author

Sadly, nothing to report yet!
For now, I would recommend downgrading final-form-arrays to the previous version.
To do that, specify the version in your package.json like so:

  "dependencies": {
    ...
    "final-form-arrays": "^1.0.4",
    ...
  }

@tlenclos
Copy link

If I do this I got the old bug, code it's quite complicated to get into :(

@tlenclos
Copy link

I had to move my form to https://github.com/jaredpalmer/formik, re-ordering is working but now we are facing performance issues.
react-final-form has just this bug preventing us from using it.

@maciejmyslinski
Copy link

I'm sorry to hear that @tlenclos 😢
I was quite busy speaking publicly recently. I'm back on it.

@maciejmyslinski
Copy link

I wrote tests for final-form-arrays and it works correctly. Maybe the error is somewhere in react-final-form-arrays 🤔
I'll try to investigate more next week.

@maciejmyslinski
Copy link

ok, I upgraded all dependencies in the codesandbox and the behavior is still buggy but slightly different. https://codesandbox.io/s/qkl20opx4w

Downgrading final-form-arrays to 1.0.4 fixes this bug so it has to be something connected with this lib 😕

@carlhueffmeier
Copy link
Contributor Author

carlhueffmeier commented Nov 26, 2018

Thank you so much for looking into it Maciej! 🙏

After reading your message I had a glance at the code again and believe I have identified the issue 🙌
The problem seems to be that in addition to the field state, we are also copying over the functions stored inside the field state. (change, blur, focus)
I will explore the issue further and make a PR once I feel confident it's fixed.

I have to admit, I was a little bit reluctant to dive into the code because debugging the issue is so involved. I ended up building the package withnps and copying the dist directory into my test repos node_modules folder.
Certainly not the ideal integration testing workflow 😅

Any hints on how to facilitate development?

carlhueffmeier added a commit to carlhueffmeier/final-form-arrays that referenced this issue Nov 27, 2018
During move and swap, the field state of the source field was copied to
the target field. The field state, however, contained functions that had
a reference to the previous position. This caused changes to be applied
to the wrong field.

Resolves: final-form#15
See also: final-form#10
@maciejmyslinski
Copy link

@carlhueffmeier thanks! This library lacks integration tests with others from the ecosystem. I'll write such integration tests soon because it's not the first time a similar issue happens.

erikras pushed a commit that referenced this issue Dec 10, 2018
During move and swap, the field state of the source field was copied to
the target field. The field state, however, contained functions that had
a reference to the previous position. This caused changes to be applied
to the wrong field.

Resolves: #15
See also: #10
@erikras
Copy link
Member

erikras commented Dec 10, 2018

Published fix in v1.1.1.

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

No branches or pull requests

4 participants