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

.submit() breaks when used on a form that has been cloned with React.cloneElement() in v8.x #4490

Open
Asday opened this issue Jun 19, 2019 · 6 comments
Labels
docs unclear-clarification-required needs more details and explanation Version 8

Comments

@Asday
Copy link

Asday commented Jun 19, 2019

Are you submitting a bug report or a feature request?

Bug report.

What is the current behavior?

Attempting to submit a form that has been cloned with React.cloneElement() fails with this.ref.current.submit is not a function.

class Container extends Component {
  renderFirstChild = () => {
    const firstChild = Children.toArray(this.props.children)[0];

    return cloneElement(firstChild, {
      ref: ref => {
        this.child = ref;
      }
    });
  };

  render = () => (
    <div>
      {this.renderFirstChild()}
      <button type="button" onClick={() => this.child.submit()}>
        Submit
      </button>
    </div>
  );
}

const SimpleForm = reduxForm({ /* ... */ })

/* ... */

<Container><SimpleForm /></Container>

What is the expected behavior?

(In my sandbox's case) the contents of "First Name" should be logged to the console upon clicking submit.

Sandbox Link

https://codesandbox.io/embed/redux-form-simple-example-quyv7

What's your environment?

react 16.5.0, redux 4.0.1, redux-form 8.2.4, react-redux 5.0.7.

Discovered the issue in Expo SDK 32, but reproduced in a more vanilla environment as shown in the sandbox.

Other information

Appears to work fine on redux-form v7.4.2. (Sandbox link with just the dependency changed).

To preempt "why are you doing this? That's ridiculous": I've made a wizard that's aware of redux form. When the user attempts to advance to the next step in the wizard, the wizard checks whether the current step has a .submit() function, and if so, freezes the wizard interface until that function's returned promise resolves, at which point it moves on to the next step.

This is done with the following code (please excuse the react native):

  renderCurrentStep = () => {
    const { children, currentStepName } = this.props

    const step = find(
      Children.toArray(children),
      (child) => child.props.stepName === currentStepName,
    )

    return isUndefined(step)
      ? null
      : cloneElement(step, { ref: (ref) => { this.currentStep = ref } })
  }

  render = () => {
    const {
      id
      lastStep,
      nextStepName,
      onNext,
      waitingToAdvance,
    } = this.props

    return (
      <View style={{ flex: 1 }}>
        <ScrollView>{this.renderCurrentStep()}</ScrollView>
          {
            !lastStep && (
              <Button
                title={`${nextStepName} >`}
                onPress={() => onNext(id, this.currentStep)}
                disabled={waitingToAdvance}
              />
            )
          }
      </View>
    )
  }

Where onNext() is this action creator, bound:

import { isFunction } from 'lodash'

export const shouldWaitToAdvance = (step) => step && isFunction(step.submit)
export const {
  REQ_NEXT,
  REQ_NEXT_FAILURE,
  REQ_NEXT_SUCCESS,
} = /* ... */
export const nextStep = (id, step) => {
  if (!shouldWaitToAdvance(step)) {
    return { type: REQ_NEXT_SUCCESS, payload: { id } }
  }

  return async (dispatch, getState) => {
    dispatch({ type: REQ_NEXT, payload: { id } })

    try {
      await step.submit()
    } catch (error) {
      dispatch({ type: REQ_NEXT_FAILURE, payload: { id }, error })

      return
    }

    dispatch({ type: REQ_NEXT_SUCCESS, payload: { id } })
  }
}

This worked fine with my test case in which I imitated a redux form:

class StepWithDelay extends Step {
  submit = () => delayAsync(1333)

  render = () => <View {...this.props} />
}

But when I came to use a redux form for real, heavens crashed to the Earth, madness reigned, and milk was spilt.

This is actually stopping my work, so once I've opened this issue, I'll be working on it myself, but I can make no promises of fixing it, because I am a stupid stupid idiot and I am very bad at things.

@Asday
Copy link
Author

Asday commented Jun 19, 2019

I wrote up a test I expected to be red, but it passed just fine.

diff --git a/src/__tests__/reduxForm.spec.js b/src/__tests__/reduxForm.spec.js
index 0df393c..3a0bc63 100644
--- a/src/__tests__/reduxForm.spec.js
+++ b/src/__tests__/reduxForm.spec.js
@@ -1,6 +1,6 @@
 import { noop } from 'lodash'
 /* eslint react/no-multi-comp:0 */
-import React, { Component } from 'react'
+import React, { Children, Component, cloneElement } from 'react'
 import TestUtils from 'react-dom/test-utils'
 import { Provider } from 'react-redux'
 import { combineReducers as plainCombineReducers, createStore } from 'redux'
@@ -5715,6 +5715,55 @@ const describeReduxForm = (name, structure, combineReducers, setup) => {
       expect(propsAtNthRender(formRender, 1).valid).toBe(false)
       expect(propsAtNthRender(formRender, 2).valid).toBe(true)
     })
+
+    it('can be submitted by instance function after being cloned', () => {
+      const store = makeStore({})
+      const Form = () => <div />
+      const onSubmit = jest.fn()
+      const Decorated = reduxForm({
+        form: 'testForm',
+        onSubmit,
+      })(Form)
+
+      class Container extends Component {
+        renderFirstChild = () => {
+          const firstChild = Children.toArray(this.props.children)[0]
+
+          return cloneElement(
+            firstChild,
+            {
+              ref: ref => { this.child = ref },
+            },
+          )
+        }
+
+        render() {
+          return (
+            <div>
+              {this.renderFirstChild()}
+              <button
+                type="button"
+                onClick={() => { console.log(this.child.submit()) }}
+              >
+                Submit
+              </button>
+            </div>
+          )
+        }
+      }
+
+      const dom = TestUtils.renderIntoDocument(
+        <Provider store={store}>
+          <Container>
+            <Decorated />
+          </Container>
+        </Provider>
+      )
+      const submit = TestUtils.findRenderedDOMComponentWithTag(dom, 'button')
+      TestUtils.Simulate.click(submit)
+
+      expect(onSubmit).toHaveBeenCalledTimes(1)
+    })
   })
 }

I was able to get it to fail like so:

diff --git a/package.json b/package.json
index 0d9fdf4..9325df3 100644
--- a/package.json
+++ b/package.json
@@ -126,7 +126,7 @@
     "prettier-eslint-cli": "^4.7.1",
     "react": "^16.7.0",
     "react-dom": "^16.7.0",
-    "react-redux": "^6.0.0",
+    "react-redux": "^5.0.7",
     "redux": "^4.0.1",
     "redux-immutablejs": "^0.0.8",
     "rifraf": "^2.0.3",

But that caused a lot of other errors:

image

At this point I noticed the following in package.json:

  "peerDependencies": {
    ...
    "react-redux": "^6.0.0 || ^7.0.0",
    ...
  },

Which is a problem. I don't want to use react-redux v6. It has performance issues which quickly necessitated the release of v7, which I can't use, because the Expo SDK I'm using, (latest at the time I started the project), doesn't have support for hooks, which react-redux v7 needs.

Needless to say, I'm in way over my head.

@Asday
Copy link
Author

Asday commented Jun 20, 2019

react-redux v7 demands react v16.8.4 minimum, but Expo SDK33 requires exactly react v16.8.3. I looked, and even though there's a lot of code changed in react between those two versions, supposedly the only thing that changed functionality-wise is a bugfix to do with dev tools.

I upgraded to Expo SDK33 (which was a colossal pain in the butt), upgraded to react-redux v7.1.0 (which was painless), and everything works ok now.


I'm going to leave this issue open at your whim because I still believe something is broken. The required react-redux version for redux-form isn't documented anywhere I could see outside of its package.json, which for sure needs fixing.

I also think not supporting react-redux v5.x was... Unfortunate. Up until Expo SDK33 was released, there was no way for me to use react-redux v7.x, and as mentioned, v6.x had performance issues. Now it's been released, I can't immediately think of a reason to have to support v5.x, but if one materialises, I think work should be considered to make it work. It looks like a lot of the failing tests were caused by similar/the same issues, so it mightn't be that much work.

@renatoagds
Copy link
Contributor

@Asday this is related with #4465 ? or not? If yes, could you keep this in one issue?

@renatoagds renatoagds added docs unclear-clarification-required needs more details and explanation labels Jun 26, 2019
@Asday
Copy link
Author

Asday commented Jun 26, 2019

That's a fantastic question; when I opened #4465 I still wasn't aware of the requirement of react-redux >=v5.

I'll retest #4465 with an upgraded react-redux version tomorrow while I make that sandbox for #4494.

@iamandrewluca
Copy link
Member

iamandrewluca commented Apr 15, 2020

@Asday any updates?

@Asday
Copy link
Author

Asday commented Apr 15, 2020

Not at all - I ended up not using normalisation at all because I had a project to ship. It's possible I revisit this while I'm furloughed, but don't depend on me for it. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs unclear-clarification-required needs more details and explanation Version 8
Projects
None yet
Development

No branches or pull requests

3 participants