Add `submitting` state property that is `true` while `onSubmit` is pending #449

BudgieInWA · 2018-07-17T06:59:26Z

This PR closes #441.

This is my attempt to implement the feature tracked in #441 in uniforms core. I am looking forward to feedback, and if it turns out that this solutions would be acceptable, I can implement validating similarly.

The commit messages provide motivation for the individual changes.

- The successs and failure callback behaviour was not being tested when submitting without `onSubmit`. - The value passed to the `onSubmit` callbacks was not tested.

A new state property `submitting` is introduced, which is `true` while an onSubmit promise is pending and `false` otherwise.

The presence of `onSubmitFailure` makes it look like it might be ok to let `onSubmit` throw an error (as well as rejecting the promise it returns). If `onSubmit` does throw an error, the form itself could enter an invalid state and the uncaught error message would be mixed up with uniforms implementation, so it would be hard for the end user to debug.

codecov · 2018-07-17T07:05:51Z

Codecov Report

Merging #449 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #449   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         157    157           
  Lines        1404   1414   +10     
=====================================
+ Hits         1404   1414   +10

Impacted Files	Coverage Δ
packages/uniforms/src/filterDOMProps.js	`100% <ø> (ø)`	⬆️
packages/uniforms/src/ValidatedForm.js	`100% <100%> (ø)`	⬆️
packages/uniforms/src/BaseForm.js	`100% <100%> (ø)`	⬆️
packages/uniforms-unstyled/src/DateField.js	`100% <0%> (ø)`	⬆️
packages/uniforms-antd/src/SelectField.js	`100% <0%> (ø)`	⬆️
packages/uniforms-bootstrap3/src/DateField.js	`100% <0%> (ø)`	⬆️
packages/uniforms-material/src/SelectField.js	`100% <0%> (ø)`	⬆️
packages/uniforms/src/JSONSchemaBridge.js	`100% <0%> (ø)`	⬆️
packages/uniforms-bootstrap4/src/DateField.js	`100% <0%> (ø)`	⬆️
packages/uniforms-antd/src/DateField.js	`100% <0%> (ø)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5171be3...056f084. Read the comment docs.

radekmie

Two minor things and one bigger one.

Great work with the tests!

packages/uniforms/src/BaseForm.js

packages/uniforms/src/filterDOMProps.js

…ing the pattern used with `changed`

It turns out the pollyfil that we are using supports it.

This reverts commit 53a9690.

This reverts commit bed378d.

radekmie · 2018-07-19T16:44:03Z

packages/uniforms/__tests__/BaseForm.js

@@ -66,7 +66,6 @@ describe('BaseForm', () => {
            expect(context.uniforms).toHaveProperty('state', expect.any(Object));
            expect(context.uniforms.state).toHaveProperty('changed', false);
            expect(context.uniforms.state).toHaveProperty('changedMap', {});
-            expect(context.uniforms.state).toHaveProperty('submitting', false);


I think that this test is still needed (or at least it's a nice to have).

radekmie · 2018-07-19T16:45:29Z

Besides that last comment, I really like it. You'd like to add the validating state in the same PR, right?

packages/uniforms/src/BaseForm.js

BudgieInWA · 2018-07-20T06:32:48Z

I will be implementing validating too. I'm happy for this PR to be merged to put a lid on the points discussed here, or to keep working here to keep the discussion together.

radekmie · 2018-07-20T07:53:59Z

So let's work on it here. I'll think about the changelog and documentation.

The existing collection of these tests was almost impossible to follow.

radekmie · 2018-07-31T18:06:10Z

Ping @BudgieInWA.

BudgieInWA · 2018-09-11T10:27:03Z

@radekmie Sorry about the delay and the git history. Here is my crack at validating state.

I think submitting an AutoForm with an async onValidate and onSubmit will go through these states:

submitting: false, validating: true
submitting: true, validating: false
submitting: false, validating: false

I don't know if it would be a better API if it went through these states:

submitting: true, validating: true
submitting: true, validating: false
submitting: false, validating: false

radekmie · 2018-09-19T13:55:02Z

Indeed, this is not so good. There's one more thing: should the validating change, if it was not triggered by submit? One can call validate manually, then it should be separate.

I think it should work, by adding submitting: true to Validated#onSubmit, next to validate: true - BaseForm should handle setting down of this flag. It's not completely OK, as they should be independent.

Opinions?

(sorry for the delay)

BudgieInWA · 2018-09-21T02:11:44Z

I think we want to define these flags as follows for easiest use:

validating is true while async work is being done and we are expecting a validation result.
submitting is true while async work is being done and we are expecting a submission result.

In that case, ValidatedForm#validate should set just validating and ValidatedForm#onSubmit should (atomically) set validating and submitting. If validation fails, it should (atomically) set them both down. If validation succeeds, it should set validating down, then proceed with the submit, which should set submitting down. (We need to add that guarantee to BaseForm#onSubmit.)

I think it's fine that ValidatedForm#onSubmit has this complicated interaction with the submitting flag, because it really is changing what "submit" means.

This seems to line up with your take, so I will start building.

(we're not in a hurry 😄)

radekmie · 2018-11-12T11:00:52Z

Ping @BudgieInWA.

BudgieInWA · 2018-11-14T03:14:59Z

@radekmie, how's that?

radekmie · 2018-11-14T08:33:57Z

packages/uniforms/src/ValidatedForm.js

+            .catch(() => {
+                // `onSubmit` should never reject, so we ignore this rejection.
+            })
+            .finally(() => {


If we are after catch, then would be better here.

radekmie · 2018-11-14T08:35:13Z

packages/uniforms/src/ValidatedForm.js

@@ -33,6 +33,8 @@ const Validated = parent => class extends parent {
        ]).isRequired
    };

+    // TODO add `uniforms.state.validating` to `childContextTypes`.


One more thing.

It is not obvious how this should be done. Unlike the actual state, with getChildContextState, the static childContextTypes is not assembled in this way. What we need is

static childContextTypes = merge( { uniforms: { state: { validating: PropTypes.bool.isRequired } } }, parent.childContextTypes );

but I don't know of any such merge function. Do we need to store the prop types nested in plain objects as well? Then we can have a shapify function that turns the plain object tree into a PropTypes.shapeOf tree for assignment to childContextTypes and the extending classes can merge the plain objects (like lodash.merge would) before assigning their own childContextTypes.

Or am I missing something simple?

There is a way to merge them, but differently - validate it twice. Prop types are just functions, so we can create a new one, which will start with calling the old one.

For now, I'd export an object from the BaseForm, marked as temporary (with comment and underscore prefixed name).

3640cde is my take on exposing a PropTypes object that can be extended. I'm happy for you to rename and move stuff around, maybe refactor shapify and BaseForm.shapifyPropTypes into a file or util library.

Looks good, from the functional level. I'd rather not have shapifyPropTypes at all (but use shapify directly) and put plainChildContextTypes in an export (again, underscored) not to pollute BaseForm.

Should I do it, @BudgieInWA?

radekmie · 2018-11-14T08:36:17Z

@BudgieInWA it looks great! Just two minor things.

radekmie · 2018-11-25T16:02:37Z

It took some time, but we finally have it! Thanks again, @BudgieInWA!

BudgieInWA · 2018-11-26T02:09:39Z

packages/uniforms/src/ValidatedForm.js

-        static childContextTypes = parent.shapifyPropTypes(plainChildContextTypes);
+    static childContextTypes = {
+        ...parent.childContextTypes || {},
+        uniforms: childContextTypes


This is awkward because parent.childContextTypes.uniforms is replaced with BaseForm.childContextTypes.uniforms (through childContextTypes) no matter what parent is. That is why BaseForm.plainChildContextTypes was a part of BaseForm (export __childContextTypesBuild is still better than BaseForm.shapifyPropTypes).

I know. There’s a small chance, that it happen not to be the case. Anyway, I’m sure it’s not really important, as the new context API doesn’t have this problem (and I’m thinking about it more and more....).

BudgieInWA added 6 commits July 17, 2018 14:27

Reformat initial state in tests in preparation for expansion

1533cfb

Add additional checks to existing tests to increase coverage

407efef

- The successs and failure callback behaviour was not being tested when submitting without `onSubmit`. - The value passed to the `onSubmit` callbacks was not tested.

Add straightforward tracking of submitting state when submitting

099ca9a

A new state property `submitting` is introduced, which is `true` while an onSubmit promise is pending and `false` otherwise.

Skip setting submitting state when onSubmit is not async.

1b4e727

Add submitting state to docs

b7df3e0

BudgieInWA requested a review from radekmie as a code owner July 17, 2018 06:59

Add test case for onSubmit throwing

bed378d

radekmie suggested changes Jul 17, 2018

View reviewed changes

packages/uniforms/src/BaseForm.js Outdated Show resolved Hide resolved

packages/uniforms/src/BaseForm.js Show resolved Hide resolved

packages/uniforms/src/filterDOMProps.js Outdated Show resolved Hide resolved

radekmie added the Type: Feature New features and feature requests label Jul 17, 2018

BudgieInWA added 7 commits July 18, 2018 13:55

Fix ordering of new filterDOMProps unwantedProp

b65eff0

Initialise submitting to false from the beginning instead of follow…

cb10d5e

…ing the pattern used with `changed`

Use promise finally instead of then

70cc1e8

It turns out the pollyfil that we are using supports it.

Rename res to result

89668b8

Revert "Catch errors (synchronously) thrown by onSubmit"

a81fbb2

This reverts commit 53a9690.

Revert "Add test case for onSubmit throwing"

be04d79

This reverts commit bed378d.

Include reason in comment about conditionally setting submitting state

5bfc060

radekmie reviewed Jul 19, 2018

View reviewed changes

packages/uniforms/src/BaseForm.js Show resolved Hide resolved

Re-add check for submitting in the default context test

6b839a9

BudgieInWA added 3 commits July 20, 2018 16:13

Rewrite tests to do with changing props

4eee44a

The existing collection of these tests was almost impossible to follow.

Re-organise and -write tests, grouping them by event

edefb1d

Add tests around interactions with schema validator

2d38fc5

BudgieInWA added 2 commits September 8, 2018 09:19

Reformat and reorder

43b0fa1

Expand prop changing tests

86690f2

BudgieInWA added 11 commits September 11, 2018 17:33

Add tests for validating context attribute

3515b43

Include validating state in ValidatedForm childContext

033c963

Merge remote-tracking branch 'upstream/master' into submitting-state-1

e680480

Merge branch 'validated-form-tests' into submitting-state-1

7c51b6e

Revert .gitignore changes

9e9aa69

Add validating state to the example SubmitField impl

551f053

Add validating state to __reset

0d529d1

Add TODO about childContextTypes

50ff7d5

Add validating to filterDOMProps.js

639ea96

Save on a Promise construction in the async case

83d6ba2

Remove mistaken line in getNativeFormProps

ae584c9

Make onValidate#onSubmit set submitting true (then false)

29f8ed8

radekmie suggested changes Nov 14, 2018

View reviewed changes

BudgieInWA and others added 4 commits November 15, 2018 15:13

Change .catch().finally() to .catch().then()

d2dd32a

Add static plainChildContextTypes to Forms, for use when extending

3640cde

Remove unused name and fix indentation

0e66889

Reorganized context types.

056f084

radekmie approved these changes Nov 25, 2018

View reviewed changes

radekmie merged commit 51da666 into vazco:master Nov 25, 2018

BudgieInWA commented Nov 26, 2018

View reviewed changes

sabativi mentioned this pull request Jan 15, 2019

Cannot get 'submitting' state from context #506

Closed

radekmie mentioned this pull request Mar 5, 2021

v3: no longer possible to know whether a form has been submitted #889

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `submitting` state property that is `true` while `onSubmit` is pending #449

Add `submitting` state property that is `true` while `onSubmit` is pending #449

BudgieInWA commented Jul 17, 2018 •

edited

Loading

codecov bot commented Jul 17, 2018 •

edited

Loading

radekmie left a comment

radekmie Jul 19, 2018

radekmie commented Jul 19, 2018

BudgieInWA commented Jul 20, 2018

radekmie commented Jul 20, 2018

radekmie commented Jul 31, 2018

BudgieInWA commented Sep 11, 2018 •

edited

Loading

radekmie commented Sep 19, 2018 •

edited

Loading

BudgieInWA commented Sep 21, 2018

radekmie commented Nov 12, 2018

BudgieInWA commented Nov 14, 2018

radekmie Nov 14, 2018

radekmie Nov 14, 2018

BudgieInWA Nov 15, 2018

radekmie Nov 15, 2018

BudgieInWA Nov 19, 2018

radekmie Nov 19, 2018

radekmie commented Nov 14, 2018

radekmie commented Nov 25, 2018

BudgieInWA Nov 26, 2018

radekmie Nov 26, 2018

Add submitting state property that is true while onSubmit is pending #449

Add submitting state property that is true while onSubmit is pending #449

Conversation

BudgieInWA commented Jul 17, 2018 • edited Loading

codecov bot commented Jul 17, 2018 • edited Loading

Codecov Report

radekmie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radekmie commented Jul 19, 2018

BudgieInWA commented Jul 20, 2018

radekmie commented Jul 20, 2018

radekmie commented Jul 31, 2018

BudgieInWA commented Sep 11, 2018 • edited Loading

radekmie commented Sep 19, 2018 • edited Loading

BudgieInWA commented Sep 21, 2018

radekmie commented Nov 12, 2018

BudgieInWA commented Nov 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radekmie commented Nov 14, 2018

radekmie commented Nov 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `submitting` state property that is `true` while `onSubmit` is pending #449

Add `submitting` state property that is `true` while `onSubmit` is pending #449

BudgieInWA commented Jul 17, 2018 •

edited

Loading

codecov bot commented Jul 17, 2018 •

edited

Loading

BudgieInWA commented Sep 11, 2018 •

edited

Loading

radekmie commented Sep 19, 2018 •

edited

Loading