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

Feature Request: onSubmit callback or onSubmitSuccess/onSubmitFailure events #51

Closed
zeroasterisk opened this issue Jun 28, 2016 · 22 comments
Assignees
Labels
Type: Feature New features and feature requests Type: Question Questions and other discussions

Comments

@zeroasterisk
Copy link
Contributor

If I am submitting a form via a method call (for example) and it's async --- I would like the ability to pass down a callback and act on it's response.

The new API for formRef.submit() is great, but I can not see any way to act on results from that submit.

Here's my current use case:

  1. Normal submit the form, saves a document and shows a success notification, doesn't navigate
  2. Clicking on a Save & Continue button, should submit the form, and on success, navigate to the next page.

The problem is, I can trigger formRef.submit() but I have no way to wait on the response of it, from the React component level, without involving internal state.

Here is what I'm doing now to handle this:

  constructor(props) {
    super(props);
    this.state = { afterSaveContinue: false };
    this.onSubmit = this.doSubmit.bind(this);
  }
  doSubmit(data) {
    const { afterSaveContinue } = this.state;
    const { myDocument, handleSubmit } = this.props;
    if (!afterSaveContinue) {
      return handleSubmit(data);
    }
    return handleSubmit(data, (err, res) => {
      if (err) return;
      browserHistory.push(`/myDocument/${myDocument._id}/nexpage`);
    });
  }
  setAsClicked() {
    this.setState({ afterSaveContinue: true });
  }

But, I could imagine enabling some alternate submit-like handler, when clicking on the "continue" button, which passed either extra arguments into our onSubmit function, or received standard events or callbacks from it.

@radekmie radekmie added Type: Feature New features and feature requests Type: Question Questions and other discussions labels Jun 28, 2016
@radekmie radekmie self-assigned this Jun 28, 2016
@radekmie
Copy link
Contributor

Yep, that's a quite common use case. How about simply doing a return at the end of onSubmit? This could suffice your needs because your doSubmit could return a Promise or something similar.


I don't really get your idea with onSubmitSuccess / onSubmitFailure - could you describe it more precisely?

@zeroasterisk
Copy link
Contributor Author

I typed in a great comment on this last week... but never submitted it and I don't feel like re-typing :/

I think the idea of a doSubmit returning a Promise and resolving... but I honestly don't know how to do that...

Can you provide an example of: onSubmit, sets the button to disabled until submit handler resolves?

@radekmie
Copy link
Contributor

radekmie commented Jul 6, 2016

Introduction

Let's assume, that BaseForm#onSubmit returns props.onSubmit(...). Then consider the following example:

class Example extends React.Component {
    constructor (props) {
        this.onSubmit     = this.onSubmit.bind(this);
        this.onSubmitNext = this.onSubmitNext.bind(this);
    }

    onSubmit (data) {
        return new Promise((resolve, reject) => {
            this.props.handleSubmit(data, (error, response) =>
                error
                    ? reject(error)
                    : resolve(response)
            );
        });
    }

    onSubmitNext () {
        // This will be either:
        //   - a Promise (success)
        //   - undefined (validation error)
        const promise = this.form.submit();

        if (promise) {
            promise.then(
                response => console.log('OK', response),
                error    => console.log('NO', error)
            );
        }
    }

    render () {
        return (
            <main>
                <AutoForm ref={form => this.form = form} onSubmit={this.onSubmit} />

                <button onClick={this.onSubmitNext}>
                    Save & Continue
                </button>
            </main>
        );
    }
}

Fail

This would work perfectly fine, but... ValidatedForm#validate calls onSubmit in a setState callback, so it's asynchronous (it would work in BaseForm and QuickForm).

Solution

Simply - Promise. BaseForm#submit will return a Promise - it will be instantly resolved in BaseForm and QuickForm and resolved with returned value of props.onSubmit(...) (or rejected with validation error) asynchronously in ValidatedForm, ValidatedQuickForm and most importantly - AutoForm. Example:

// onSubmit returns something synchronously or returns a Promise
const Example = ({onSubmit, onSubmitSuccess, onSubmitFailure}) => {
    let ref;

    return (
        <main>
            <AutoForm ref={ref => form = ref} onSubmit={onSubmit} />

            <button onClick={() => ref.submit().then(onSubmitSuccess, onSubmitFailure)}>
                Save & Continue
            </button>
        </main>
    );
};

Note: This won't work with onSubmit = (model, callback) => {...}.

@zeroasterisk
Copy link
Contributor Author

Yup - this looks like it's a perfect fit... I'll try it out and let you know if I run into problems... This is very much worth including in docs/examples, as it's a very common use case... you rule!

Bonus points:
Out of curiosity, is it possible to do this same pattern without using ref={ref => form = ref} and the button? If I have a form without a submit button (enter to submit, or autosave) --- can I still wrap submission in a Promise and handle the resolve elsewhere?

@radekmie
Copy link
Contributor

radekmie commented Jul 6, 2016

I'll reopen this, because... There's no returned value from form.submit right now - it has to be implemented.

I don't clearly understand, what you mean - could you develop this idea a little?

@radekmie radekmie reopened this Jul 6, 2016
@zeroasterisk
Copy link
Contributor Author

I think I'm trying to make your first example work... without setting the state of the ref to the form... Trying to make the promise handling all abstract from the rendered form UI...

class Example extends React.Component {
    constructor (props) {
        this.state = { submitting: false, success: false, failure: false };
        this.onSubmit = this.onSubmit.bind(this);
    }
    onSubmit(data) {
        this.setState({ submitting: true });
        const promise = this.onSubmitMakePromise(data);
        if (promise) {
            promise.then(
                response => this.setState({ submitting: false, success: true })
                error    => this.setState({ submitting: false, failure: true })
            );
        }
    }
    onSubmitMakePromise(data) {
        return new Promise((resolve, reject) => {
            this.props.handleSubmit(data, (error, response) =>
                error ? reject(error) : resolve(response)
            );
        });
    }
   ...
}

@radekmie
Copy link
Contributor

radekmie commented Jul 7, 2016

No, @zeroasterisk - you are not setting state of the ref to the form. That ref is required because we have to submit that form (because form.submit() will return a Promise). Basically, it's completely UI-agnostic to some extent - we have to know, which form we submit, so we can react on it. If you want to do so without this ref, there should be 2 more props - something like onSubmitSuccess and onSubmitError.

@radekmie
Copy link
Contributor

radekmie commented Jul 7, 2016

Of course, we can do both - Promise will be returned from BaseForm#submit, but also both onSubmitSuccess and onSubmitError will be automatically used - .then(props.onSubmitSuccess, props.onSubmitError).

@serkandurusoy
Copy link
Contributor

I think we are getting closer every day to a point where we'll be needing form lifecycle hooks :)

@radekmie
Copy link
Contributor

To be honest, I've just finished it few minutes ago and wanted to push it, but new bugs appeard, so it will wait.


Probably yes, @serkandurusoy - kind of.

@radekmie
Copy link
Contributor

radekmie commented Jul 20, 2016

Example:

// New props
<AutoForm
    schema={schema}
    onSubmit={model => db.saveThatReturnsPromise(model)}
    onSubmitSuccess={() => alert('Saved!')}
    onSubmitFailure={() => alert('Error!')}
/>

// New API
baseFormRef.submit().then(
    () => console.log('Submitted'),
    () => console.log('Submit error')
)

validatedFormRef.submit().then(
    () => console.log('Submitted'),
    () => console.log('Invalid or submit error')
)

To summarize current (new) API changes:

  • (added) onSubmitSuccess property to BaseForm
  • (added) onSubmitFailure property to BaseForm
  • (changed) onSubmit (and so submit) is now asynchronous and returns a Promise
  • (changed) onValidate (and so validate) is now asynchronous and returns a Promise
  • (changed) onValidateModel (and so validateModel) is now asynchronous and returns a Promise

How it worked:

  • HTML submit event
  • validation
  • validation (onValidate)
  • if no error occured or reported
    • submit (onSubmit)

How it works now:

  • HTML submit event
  • validation
  • validation (onValidate)
  • if no error occured or reported
    • submit (onSubmit)
      • additional onSubmitSuccess is attached (.then)
      • additional onSubmitFailure is attached (.catch)

Planned changes:

  • onValidate have to return a Promise

Any feedback appreciated.

@serkandurusoy
Copy link
Contributor

@radekmie Just so to make it clear, is this breaking change?

Do existing apps on rc26 need to change their existing onSubmit handlers moving forward?

@radekmie
Copy link
Contributor

Only, if you were programmatically calling formRef.submit().

@serkandurusoy
Copy link
Contributor

Hm, as much as I appreaciated the promise based API here, does this not introduce an incosistency to how submit event is getting handled?

@radekmie
Copy link
Contributor

I don't get your point. Why?

@serkandurusoy
Copy link
Contributor

Well, my train of thought is

onSubmit => handler passed the doc
ref.submit() => submit => promise => then => handler passed the doc

I would expect onSubmit and ref.submit produce the same output.

@radekmie
Copy link
Contributor

Internally a Promise is created anyway (so onSubmitSuccess and onSubmitFailure could work as intended).

@serkandurusoy
Copy link
Contributor

Hm I don't quite understand if onSubmitSuccess is resolved with the model/doc as its argument so we can act on it.

It it does, in that case, onSubmit becomes redundant, if it does not, I would very much say deprecating onSubmit would then be a better idea for sake of future opportunities.

@radekmie
Copy link
Contributor

No, onSubmitSuccess is resolved with result of onSubmit. I wouldn't say so. It goes like this: "I'll do a save in onSubmit and care about it in onSubmitSuccess/onSubmitFailure".

@serkandurusoy
Copy link
Contributor

serkandurusoy commented Jul 21, 2016

Oh, so if the onSubmit prop does not return anything, it is fine, but if it does, it should return a promise that would be resolved to or rejected to either one of onSubmitSuccess or onSubmitFailure, right?

So in essence, ref.submit() still fires off the handler for onSubmit but what this new feature brings in is the ability tap into the result of the handler from outside of the handler function.

Do I get it right?

@radekmie
Copy link
Contributor

Yes, indeed.

@zeroasterisk
Copy link
Contributor Author

I'm impressed - i think this is a very elegant solution...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features and feature requests Type: Question Questions and other discussions
Projects
Archived in project
Development

No branches or pull requests

3 participants