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

make onSubmit/submitForm async #1229

Closed
wants to merge 2 commits into from
Closed

Conversation

devjones
Copy link

@devjones devjones commented Jan 8, 2019

This PR enables chained promises with the submitForm imperative method. It will wait until any optional promises returned by a form's handleSubmit is resolved, before executing a chained promise. Such as:

submitForm().then(response=> {
  doSomethingAfterSubmit();
})

The async implementation in v2.0 canary does not support expected behavior of chainable promises since the promise in that function is never returned. Therefore in that implementation, doSomethingAfterSubmit() does not wait for resolution of a handleSubmit promise.

See below for the impact of return statements in Promise handler functions:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then#Return_value

@devjones devjones mentioned this pull request Jan 8, 2019
@flaviouk
Copy link

Hey, any updates on how this is going?

@Nemsae
Copy link

Nemsae commented Jan 25, 2019

Noob question: how can I pull this branch?

@devjones
Copy link
Author

@Nemsae Asa temporary stopgap until a fix is merged, you can clone my fork.

Then run

  1. yarn link in the cloned repo
  2. yarn link formik in your project folder.

This will force your project to use the forked version.

@Nemsae
Copy link

Nemsae commented Jan 28, 2019

@devjones Appreciate the help but I have an issue using your forked repo. The file/folder structure between your repo that I run yarn link within differs greatly with the formik folder installed in node_modules with npm i --save formik.

Namely the index.js file in your fork is referencing formik.cjs.development.js which doesn't exist.

I get this error after running yarn link formik in my project folder:

Module not found: Error: Can't resolve './formik.cjs.development.js' in '/Users/Jcompsta/temp/Martin/formik'

I feel I may be missing a step.

@devjones
Copy link
Author

@Nemsae You also need to build the package w/ yarn start. Follow Jared's guide here: https://github.com/jaredpalmer/formik/blob/master/.github/CONTRIBUTING.md

@jaredpalmer
Copy link
Owner

jaredpalmer commented Feb 1, 2019

Fix conflicts por favor

@jaredpalmer jaredpalmer self-assigned this Feb 1, 2019
@devjones
Copy link
Author

devjones commented Feb 2, 2019

@jaredpalmer Conflicts resolved.

@@ -445,16 +445,17 @@ export class Formik<Values = FormikValues> extends React.Component<
this.setState({ isValidating: false });
const isValid = Object.keys(combinedErrors).length === 0;
if (isValid) {
this.executeSubmit();
return Promise.resolve(this.executeSubmit());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why wrap this with Promise.resolve? Isn't the result the same without it?

Choose a reason for hiding this comment

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

This guarantees that a promise is returned. If this.props.onSubmit were to return void instead of a promise, this would ensure chaining.

@@ -445,16 +445,17 @@ export class Formik<Values = FormikValues> extends React.Component<
this.setState({ isValidating: false });
const isValid = Object.keys(combinedErrors).length === 0;
if (isValid) {
this.executeSubmit();
return Promise.resolve(this.executeSubmit());

Choose a reason for hiding this comment

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

This guarantees that a promise is returned. If this.props.onSubmit were to return void instead of a promise, this would ensure chaining.

} else if (this.didMount) {
// ^^^ Make sure Formik is still mounted before calling setState
this.setState({ isSubmitting: false });
}
return;

Choose a reason for hiding this comment

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

Was this intentional to return void when there are validation errors? This would imply that consumer is checking the result of submitForm() before chaining to it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree we should rather harmonize on a single return type like Promise<Result | null>

Copy link
Owner

Choose a reason for hiding this comment

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

Promise<Result | undefined>

@ryanburr
Copy link

Looks like the types for submitForm and onSubmit need to be updated.

https://github.com/devjones/formik/blob/master/src/types.tsx#L105
https://github.com/devjones/formik/blob/master/src/types.tsx#L202

@slorber
Copy link
Collaborator

slorber commented Mar 25, 2019

@jaredpalmer do you plan to support this long awaited feature on 1.x?

That would be helpful as people can't necessarily migrate to hooks immediately and it's not yet a stable release (I'm doing Expo/RN in my case). I would be fine with a workaround until 2.x if you have one to share, but unfortunately I don't really see how to easily reproduce the behavior inside submitForm myself

@jaredpalmer
Copy link
Owner

@slorber planning to release v2 at React Europe

@slorber
Copy link
Collaborator

slorber commented Apr 3, 2019

Great! As hooks are going in Expo 33 that should sync well

@ffMathy
Copy link

ffMathy commented Jun 1, 2019

@jaredpalmer when is React Europe? Is it the one in May 2020? Because that's a crazy long wait for something like this.

@slorber
Copy link
Collaborator

slorber commented Jun 1, 2019

@ffMathy as you can see there are RC's being published and actively worked on: https://github.com/jaredpalmer/formik/releases

@ffMathy
Copy link

ffMathy commented Jun 1, 2019

@slorber ah very cool!

Is there an ETA on a release date?

@jaredpalmer
Copy link
Owner

Working pretty actively on v2. Things are getting stable. There are few more bits to tackle (#1371 , #1560, and #1555 are the big ones). After that we need to update the docs and write new examples.

@jaredpalmer
Copy link
Owner

@ffMathy Feel free to review active PRs and contribute. We could use the help!

@jaredpalmer
Copy link
Owner

This behavior is in v2 right now btw.

@andrewMuntyan
Copy link
Contributor

Hi, all!

This behavior is in v2 right now btw.

@jaredpalmer have a question. I've updated to 2.0.1-rc.7 and trying to use this async submitForm.
It works just great in case form submission is completed without errors.
But in case I want to throw an error from my submitHandler it does not work properly.
Quick example:

// SomeContainer.tsx
submitHandler = async (
    values,
    { setErrors }: FormikHelpers<IMappedDefaultModelDataShape>,
  ) => {
    // ...some code here
    try {
      return await update(id, values);
    } catch (err) {
      throw Error(err);
    }
  };

  render() {
  return <Formik {...this.props} onSubmit={this.handleSubmit}></Formik>
 }
// SomeFieldRenderer.tsx
const onSaveButtonClick = async () => {
        const { submitForm } = props.formikActions;
        return submitForm()
          .then(promise => {
           // i'm always getting here
          })
          .catch((e: any) => {
           //cant get here
          });
      };

As I see from source code inside submitForm we handle catch case and just dispatch 'SUBMIT_FAILURE' in case submitHandler throws an error.

const submitForm = useEventCallback(() => {
    // ...some code here
    return validateFormWithHighPriority().then(
        // ...some code here
          return Promise.resolve(executeSubmit())
            .then(() => {
              if (!!isMounted.current) {
                dispatch({ type: 'SUBMIT_SUCCESS' });
              }
            })
            .catch(_errors => {
              if (!!isMounted.current) {
                dispatch({ type: 'SUBMIT_FAILURE' });
              }
             // I suppose we have to throw here in order to catch properly in my `onSaveButtonClick` method
            });
        // ...some code here
        return;
      }
    );
  }, [executeSubmit, validateFormWithHighPriority]);

Why do we not throwing error from cacth case? is it intentional?

@jaredpalmer
Copy link
Owner

@andrewMuntyan Not sure. Let's make a new issue with a repro.

@andrewMuntyan
Copy link
Contributor

@andrewMuntyan Not sure. Let's make a new issue with a repro.

here #1636 I've created a new one

@jaredpalmer
Copy link
Owner

This is now in v2.

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

Successfully merging this pull request may close these issues.

9 participants