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

submitOnChange causes issues when rendering arrays/objects #7

Open
bebraw opened this issue Jun 27, 2014 · 8 comments
Open

submitOnChange causes issues when rendering arrays/objects #7

bebraw opened this issue Jun 27, 2014 · 8 comments

Comments

@bebraw
Copy link
Contributor

bebraw commented Jun 27, 2014

I have arrays/objects in my schema. Looks like if submitOnChange is enabled, onSubmit doesn't take initial values in count. This means JSX rendering can fail in case it expects arrays/objects (initially undefined now).

You can see this problem at https://github.com/koodilehto/invoice-frontend/blob/3e1dd956fa7a1a5c57f906e78c1e21a4cb648f07/src/js/app.jsx .

I don't know what's the most natural way to deal with it. I could likely hack around it by doing some sort of copy at onSubmit to get the initial values right or hack at render but those don't feel right.

@odf
Copy link
Contributor

odf commented Jun 27, 2014

I don't think this is specific to submitOnChange being active. The form output is pruned quite aggressively, using this function. The underlying reason for this is that we want arrays not only to grow, but also to shrink automatically, so if an element of an array (which could itself be an object or array) contains no data, we want to remove it completely. The strategy for this, at the moment, is to recursively replace empty strings, arrays and object with null, and then remove any object properties or array entries whose value is null or undefined.

It should be possible to implement a more intelligent pruning process, or to add a post-processing step that puts sensible default values into remaining empty slots within the pruned data. I can look into that tomorrow. Personally, I prefer to allow null values in certain situations and catch them using something like (a || []).map(...), but I admit that it's not a pretty solution.

@bebraw
Copy link
Contributor Author

bebraw commented Jun 27, 2014

It should be possible to implement a more intelligent pruning process, or to add a post-processing step that puts sensible default values into remaining empty slots within the pruned data. I can look into that tomorrow. Personally, I prefer to allow null values in certain situations and catch them using something like (a || []).map(...), but I admit that it's not a pretty solution.

Looks like removing this check resolves the issue partially: https://github.com/AppliedMathematicsANU/plexus-objective/blob/master/index.js#L97 . So instead of defaulting to null on empty array/object you just let the array/object remain there. The problem is that after this change there can be multiple empties in an array if you have a recursive structure (say array of objects).

You can resolve this issue by implementing another pass that gets rid of the extra empties and ensures only one will remain at the end.

Do you think it would make sense to allow empties in the middle of an array? You can end up with this sort of situation if you create items to an array and then empty one in the middle through UI. The current implementation seems to prune these too. Maybe that's the right way to do it.

@odf
Copy link
Contributor

odf commented Jun 27, 2014

I think that leaving the basic pruning algorithm as it is now and then patching up the result will be simpler. But I may be missing some subtleties. I'll hopefully see clearer when I implement the fix.

By the way, I've started writing a randomised test suite for plexus-objective. Writing a specification and generating test cases for prune should be interesting. :-) I hope I can get randomised test suites going for plexus-validate and plexus-form soon, too.

I like the fact that currently, empty array items between non-empty ones are pruned away. But of course, there could be applications where this is not desired. If the array items themselves are rather complex, it might also be useful to have a way to delete the whole item with one click. So there's definitely room for extending the form specification there.

@bebraw
Copy link
Contributor Author

bebraw commented Jun 27, 2014

By the way, I've started writing a randomised test suite for plexus-objective. Writing a specification and generating test cases for prune should be interesting. :-) I hope I can get randomised test suites going for plexus-validate and plexus-form soon, too.

I've implemented some tooling that can help you with this. annofuzz is likely a good starting point. It's basically QuickCheck for JavaScript. First you'll need to annotate your function to test with some invariants (ie. type information). The tool will use this information to generate tests using the provided generators.

I like the fact that currently, empty array items between non-empty ones are pruned away. But of course, there could be applications where this is not desired. If the array items themselves are rather complex, it might also be useful to have a way to delete the whole item with one click. So there's definitely room for extending the form specification there.

What if it was possible to swap the prune mechanism? Use some sane default and allow it to be replaced with something else.

@odf
Copy link
Contributor

odf commented Jun 28, 2014

Thanks for the tip! Your annotation tools look really useful. Have you implemented randomised, model-based API testing a la Erlang QuickCheck, as well? I've dabbled with that a bit in my comfychair project, and it's looking quite promising.

Yes, I think allowing the caller to override the pruning algorithm is a good idea. The same could be done for the parsers and normalisers, while we're at it. I think of all these as in more or less in the same category as the validation function, except that they are much simpler, so it makes sense to provide default implementations.

@bebraw
Copy link
Contributor Author

bebraw commented Jun 28, 2014

Thanks for the tip! Your annotation tools look really useful. Have you implemented randomised, model-based API testing a la Erlang QuickCheck, as well? I've dabbled with that a bit in my comfychair project, and it's looking quite promising.

It is missing some of the fancy stuff (ie. shrinking which you appear to have). It just generates a bunch of tests based on annotations and the external invariant. You have control over generation (random, pseudorandom) and it's quite pluggable this way.

It comes with some performance overhead. On the other hand you get multiple dispatch and runtime warnings should some invariant fail. I've been thinking of implementing something to translate annotated code into more optimized version (just plain old ifs) but haven't bothered to do that yet.

It would be very valuable for me if you could provide some feedback on the tool. Maybe we should even merge efforts. You probably understand QuickCheck better than me. :)

@bebraw
Copy link
Contributor Author

bebraw commented Jan 6, 2015

What do you want to do with this issue?

@odf
Copy link
Contributor

odf commented Jan 6, 2015

I'll have to look at it again. I haven't touched any front end stuff in a good while and feel a bit lost at the moment. We'll be training a front end developer to take over the more concrete parts of the UI implementation, so I imagine this and similar issues will come up while I teach them how to use plexus-form.

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

No branches or pull requests

2 participants