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

Memoize transform to fix long standing useForm bug #1718

Closed
wants to merge 3 commits into from

Conversation

aviemet
Copy link

@aviemet aviemet commented Nov 6, 2023

I'm opening another PR to fix this bug (#1171, #1131, #1491, #1631)

In the React useForm package, the transform function is set as a local variable in the component. This means that if the page triggers any re-render, the transform variable is re-initialized to the placeholder function ((data) => data). For this reason, it's more likely than not that the transform method does nothing, rendering the docs inaccurate.

The previous PRs were closed without being read. I think the whole React community here would really appreciate this bug finally being fixed.

@mattsims
Copy link

@reinink Any idea when this might be merged? 🙏

@mochetts
Copy link

mochetts commented Dec 19, 2023

I need this fix as well! The transform callback is being overridden by the default (data) => data after doing something basic like form.setData.

Repro steps:

const myCallback = (data) => { 
 console.log("myCallback")
 return data
}
form.transform(myCallback)
form.post('some/url', { onSuccess: ({ props }) => {
  form.setData(props.formData)
  form.post('some/url') // "myCallback" is not printed out
})

@mochetts
Copy link

mochetts commented Dec 19, 2023

I added a package containing this fix in the npm registry if anyone is interested:
https://www.npmjs.com/package/@moraki/inertia-react

@dac514
Copy link

dac514 commented Dec 28, 2023

Yay, thanks! Now what?

@dac-humi
Copy link

Please forgive me for tagging... @claudiodekker @jessarcher @thecrypticace
Can this PR get some feedback? Thanks for your time and hard work.

@aviemet
Copy link
Author

aviemet commented Mar 12, 2024

It would sure be great to get this long standing bug fixed. Hoping this is the bump that gets attention.
@reinink

jrson83 added a commit to inertiajs-revamped/inertia that referenced this pull request Apr 18, 2024
@pedroborges
Copy link
Collaborator

I just tried to reproduce the issue described here but couldn't 🤷‍♂️ Am I missing something?

I tested with the code below on the React Playground's js/Pages/Form.tsx file:

  const form = useForm('NewUser', {
    name: '',
    company: '',
    role: '',
  })

  function submit(e) {
    e.preventDefault()
    form.transform((data) => ({ ...data, name: 'Xyz' }))
    form.post('/user')
  }
  1. Fill out the form
    Screen Shot 2024-09-13 at 12 58 19

  2. Submit it
    Screen Shot 2024-09-13 at 12 58 29

  3. Click on the reset button, and fill out the form again
    Screen Shot 2024-09-13 at 13 06 16

  4. Submit it
    Screen Shot 2024-09-13 at 13 06 21

@pedroborges
Copy link
Collaborator

By the way, it seems #1607 fixes this and some other issues.

@derrickreimer
Copy link
Contributor

Hey @pedroborges, I've done some work on this one. Fundamentally there's a race condition at play here: if a rerender triggers before the custom transform function gets called, then the custom transform will get overwritten. (More explanation here: #1491 (comment)). This is because transform is stored in a local variable in the component body, which is not a safe way to store a stable reference to something:

// This will get reset every time the component renders
let transform = (data) => data

// ...

return {
  transform(callback) {
    transform = callback
  },

My recommendation is that we merge #1607 (with thanks to @aviemet for the work here as well), since it includes this fix and some additional memoization to mitigate unnecessary re-renders.

@pedroborges
Copy link
Collaborator

Thanks @derrickreimer, I'll take your advice and close this on in favor of #1607.

Thanks for your work and patience @aviemet, I'll let you know when the other PR is merged.

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

Successfully merging this pull request may close these issues.

7 participants