Feature: Add support for validators that are promise-based or throw errors #72

peterszerzo · 2018-10-16T08:42:24Z

Why

This PR fixes #57, adding support for validators that either throw runtime errors or work through a promise, extending the support for libraries like yup.

Open questions

Errors

The errors coming from this validation are standard and can produce anything, so they don't fit into the { message: string, data: TError | string } interface - so as a first pass, I am passing a RESOLVE_ERROR string as message so it is easily recognizable, and stringifying the actual error inside data - which the end user would have to parse back into JSON later. Wonder if there is a better way to deal with this.

TejasQ · 2018-10-16T10:02:48Z

.gitignore

@@ -93,6 +93,7 @@ typings/
 public
 lib

+


Please remove this change from this PR.

TejasQ · 2018-10-16T10:08:47Z

src/Get.tsx

+    try {
+      if (resolve) {
+        const resolvedDataOrPromise: TData | Promise<TData> = resolve(data);
+        resolvedData = (resolvedDataOrPromise as { then?: any }).then


What is the reasoning behind this type assertion? Tbh it looks a little messy.

I wasn't able to get it to type-check any other way - looks like detecting whether something is a promise or a function is not that straightforward to follow by the compiler. Do you have any idea how this could improve?

I think we can pair up on it. I'd love to understand the complexity more.

resolveData = await Promise.resolve(resolve(data)) // return a TData

👼

Awesome - I managed to confirm via the parcel example that the new features are working, now it's just a matter of cleaning up the code.

TejasQ · 2018-10-18T04:57:38Z

example/README.md

@@ -0,0 +1,3 @@
+This is a simple live demo of `restul-react` components primarily used to test features.
+
+Run it by getting a global Parcel by running `npm i -g parcel`, then running `parcel example/index.html` from root.


I think we can add parcel as a devDependency and it should be fine.

Oh right, makes sense :)

fabien0102 · 2018-10-16T14:35:45Z

src/Get.test.tsx

+        </RestfulProvider>,
+      );
+
+      await wait(() => expect(children.mock.calls.length).toBe(1));


Ok, we have the children call 1 time, but what is the expected behaviour? having an error? some data? anything?

Good catch. Might make more sense to make the test a little more expressive.

fabien0102 · 2018-10-16T14:45:45Z

src/Get.tsx

+    try {
+      if (resolve) {
+        const resolvedDataOrPromise: TData | Promise<TData> = resolve(data);
+        resolvedData = (resolvedDataOrPromise as { then?: any }).then


resolveData = await Promise.resolve(resolve(data)) // return a TData

👼

fabien0102 · 2018-10-16T14:49:32Z

src/Get.tsx

+    } catch (err) {
+      resolvedData = null;
+      resolveError = {
+        message: "RESOLVE_ERROR",


I don't see this message in any test, can you add a test for this?

fabien0102 · 2018-10-19T13:29:00Z

example/README.md

@@ -0,0 +1 @@
+This is a simple live demo of `restul-react` components primarily used to test features. Run it with `npm run example`.


We already have codesandbox examples and unit tests. If you can avoid to introduce something else to maintain… And I'm sorry but this is not replacing unit tests. I can help you to add the correct unit tests for this feature if needed but please remove this.

This is just a dev-server kind of like in @operational/components. It doesn't really add much bloat to the library. It's just an on-device, in-browser player like GraphiQL. Ideally, we don't maintain this but make sure our features work in a browser context.

I think the tests:

should be maintained

add a lot of value

but, there is no guarantee that they work 100% in a browser environment with calls to real APIs. This will serve as a check we do during development to make sure things work before pushing.

I don't really see any major costs to having this around. Do you?

@peterszerzo he does have a point though – we should ideally add unit tests for the resolve behavior as well. I'm happy to add these if you like.

I see this more as an integration test as opposed to a unit test - it makes sure that a real request can be made, goes through the state management, and renders something in the end. Which is something I am not confident a unit test suite would catch, especially after a feature this complicated.

I don't dispute the need for improving the tests for the new resolve - I just don't feel comfortable shipping this feature without validating it with an end-to-end example. This could even grow to be like the visual test cases suite in https://github.com/contiamo/operational-visualizations

And at this library stage, I'm not agree with you, we must have unit tests for every features. Without tests, we can't be sure that the functionally works as expected, it's hard to maintained and everybody can broke the feature by mistake (and by everybody, I include myself of course).

I'm maybe a bit strict on this stack, but all our product depends on this library 😉

I'm just worry about the "it's works on my little example, let's skip the tests for now" 😉

As long as we have you on the team, I am confident that this will never happen. 😉 Let's also look at making the examples richer and exportable to CodeSandbox (thanks, by the way, @CompuIves) at a later stage.

@fabien0102 we don't have to merge until we have a satisfactory test suite, and that would have been the case whether we had the example server or not. Both methods of testing are good for reliability and for catching issues that can bubble up in dependent projects - there is no reason to jump against one of them.

@peterszerzo I agree that both methods of testing are good in the context of a PR, but our goal is to have a maintainable product in the long term. For this purpose, unit tests ensure a non regression in the future, it's automated in the CI, etc etc... So no, both methods of testing are not equal in this case.

peterszerzo · 2018-10-19T14:29:47Z

The unit tests have been added, testing both validation succeeds and failures in the runtime error and promise cases.

fabien0102 · 2018-10-23T13:11:28Z

I have some questions/remarks about this PR:

Why are we introducing async into the resolve function? I would prefer if we stuck to a sync function here to avoid misunderstandings of our library (for example using another fetch in this resolve function).
For validating we can use https://www.npmjs.com/package/yup#mixedvalidatesyncvalue-any-options-object-any so this keep this stack very simple and performant.
Can we save our yarn.lock and git rebase everything to avoid 2/3 huges commits that make our repository bigger for nothing?
I'm not totally against the dev server, but it's not in the scope of this issue/pr, can we open an issue to discuss a bit more about this? 👼

TejasQ · 2018-10-23T13:17:43Z

I'm not totally against the dev server, but it's not in the scope of this issue/pr, can we open an issue to discuss a bit more about this? 👼

I'm not sure this needs to be a discussion/rfc. It adds value to this and future PRs by adding an extra layer of validation in a browser environment that is a nice complement to the already great suite of unit tests we have.

Are there concrete concerns about the dev server that you have? Maybe we can have a shorter discussion about them here. 🙂

I get that it's not the scope of this PR, but it does help us test this PR and so for that reason, I think it's quite alright to leave it in here.

fabien0102 · 2018-10-23T13:23:56Z

For me it's not an extra layer of validation, it's more code to maintained, for a ui library, I want to say why not, but in our case, we don't have any browser specificities. If it's works in unit tests, it's works in the browser (if not, I want a concrete example where it's not the case 😉)

It's also not helping to test the PR, with unit tests we have travis that ensure that everything is working, with dev server, we need to pull locally, start npm run examples, and try every examples (this one is the first one, but I guess that the goal is to have more examples). Bref, really really not convince by the value of adding another peace of code here… 😕

peterszerzo · 2018-10-23T13:40:05Z

@fabien0102 some thoughts on your comments:

async makes things cleaner in some cases and necessary in others when lazy/recursive and transformation is involved. I am happy to share my use-cases to see if we can get away without this.
sure.
as we have already discussed, having a full working example in the dev server is something that helps me confidently propose this feature as an integration-tested reliable candidate for merging. In that sense, it is very much a part of this feature.

fabien0102 · 2018-10-23T13:44:00Z

@peterszerzo Perfect, I let you rebase and fix the yarn.lock and we should be able to merge 👍

TejasQ · 2018-10-23T14:22:36Z

In response to this,

it's more code to maintained

I think we have different thoughts behind how this will be used. The way I see it, this code will not be maintained because no part of it is public. Rather, I see it:

being a quick demo of the functionality the PR describes,
not changing with all PRs, but just some specific ones,
only being used in cases where we want to easily test and reproduce more real-world examples.

I think it's nice to have this as part of the "to test" list that goes into a PR for some cases that need more thorough integration testing, for the lifetime and purpose of validating their containing PR only.

I don't see much of a cost here for having this around. It's kind of like an appendix: it doesn't do much, it isn't included in the distributed package on npm, it has no measurable cost to our users, but can and does add value to our team as we develop features that might need more reassurance.

If it's works in unit tests, it's works in the browser

This is a very bold statement that I would carefully consider. Are you sure about this? There are enforcements that usually surprise us in browsers even today: invalid/non-existent TLS, HSTS, CORS, and CORB that I'm not sure our unit tests would catch using nock. The dev-server allows us to test these cases in a variety of browsers ensuring that our PRs don't come with surprising behavior in real-world contexts.

peterszerzo · 2018-10-25T07:22:54Z

Some additional findings on the need of for promise-based validation: I tried synchronously validating some schemas yesterday with yup putting multiple options inside try/catch blocks, and got inconsistent validation results because exceptions were thrown inconsistently (sometimes passing a try/catch block, sometimes not). This resulted in inconsistent data coming into the app from refresh to refresh.

TejasQ · 2018-10-30T15:05:48Z

I would factor out that default resolve into another file, but besides that I don't see any major gotchas.

peterszerzo · 2018-10-30T15:57:53Z

@TejasQ it's in src/util now.

fabien0102 · 2018-10-30T16:00:17Z

@peterszerzo Can your git rebase -i to avoid the 2/3 commits dealing with packages-lock.json and yarn.lock? 👼

peterszerzo requested review from fabien0102 and TejasQ October 16, 2018 08:42

peterszerzo changed the title ~~Add support for validators that are promise-based or throw errors~~ **Feature:** Add support for validators that are promise-based or throw errors Oct 16, 2018

TejasQ reviewed Oct 16, 2018

View reviewed changes

TejasQ reviewed Oct 18, 2018

View reviewed changes

peterszerzo force-pushed the feature/validation-handling branch 2 times, most recently from 21341d9 to 7e54f91 Compare October 18, 2018 11:31

fabien0102 suggested changes Oct 19, 2018

View reviewed changes

peterszerzo force-pushed the feature/validation-handling branch 2 times, most recently from 7b3ff91 to e5b1c61 Compare October 23, 2018 07:29

peterszerzo force-pushed the feature/validation-handling branch 3 times, most recently from b1b6a00 to c57c921 Compare October 30, 2018 14:28

TejasQ self-assigned this Oct 30, 2018

peterszerzo assigned peterszerzo and unassigned TejasQ Oct 30, 2018

TejasQ previously approved these changes Oct 30, 2018

View reviewed changes

peterszerzo dismissed TejasQ’s stale review via 413cada October 30, 2018 15:55

TejasQ previously approved these changes Oct 30, 2018

View reviewed changes

Add support for validators that are promise-based or throw errors

086f94a

peterszerzo dismissed TejasQ’s stale review via 086f94a October 30, 2018 16:11

peterszerzo force-pushed the feature/validation-handling branch from 413cada to 086f94a Compare October 30, 2018 16:11

fabien0102 approved these changes Oct 30, 2018

View reviewed changes

peterszerzo merged commit c6d9c0f into master Oct 31, 2018

peterszerzo deleted the feature/validation-handling branch October 31, 2018 09:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Add support for validators that are promise-based or throw errors #72

Feature: Add support for validators that are promise-based or throw errors #72

peterszerzo commented Oct 16, 2018

TejasQ Oct 16, 2018

TejasQ Oct 16, 2018

peterszerzo Oct 16, 2018

TejasQ Oct 16, 2018

fabien0102 Oct 16, 2018

peterszerzo Oct 18, 2018

TejasQ Oct 18, 2018

peterszerzo Oct 18, 2018

fabien0102 Oct 16, 2018

TejasQ Oct 19, 2018

fabien0102 Oct 16, 2018

fabien0102 Oct 16, 2018

fabien0102 Oct 19, 2018 •

edited

Loading

TejasQ Oct 19, 2018 •

edited

Loading

TejasQ Oct 19, 2018 •

edited

Loading

peterszerzo Oct 19, 2018

peterszerzo Oct 19, 2018

fabien0102 Oct 19, 2018

fabien0102 Oct 19, 2018

TejasQ Oct 19, 2018

peterszerzo Oct 19, 2018

fabien0102 Oct 19, 2018

peterszerzo commented Oct 19, 2018

fabien0102 commented Oct 23, 2018 •

edited

Loading

TejasQ commented Oct 23, 2018 •

edited

Loading

fabien0102 commented Oct 23, 2018 •

edited

Loading

peterszerzo commented Oct 23, 2018

fabien0102 commented Oct 23, 2018

TejasQ commented Oct 23, 2018

peterszerzo commented Oct 25, 2018

TejasQ commented Oct 30, 2018

peterszerzo commented Oct 30, 2018

fabien0102 commented Oct 30, 2018

		@@ -0,0 +1,3 @@
		This is a simple live demo of `restul-react` components primarily used to test features.

		Run it by getting a global Parcel by running `npm i -g parcel`, then running `parcel example/index.html` from root.

		@@ -0,0 +1 @@
		This is a simple live demo of `restul-react` components primarily used to test features. Run it with `npm run example`.

**Feature:** Add support for validators that are promise-based or throw errors #72

**Feature:** Add support for validators that are promise-based or throw errors #72

Conversation

peterszerzo commented Oct 16, 2018

Why

Open questions

Errors

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

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

fabien0102 Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

TejasQ Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

TejasQ Oct 19, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterszerzo commented Oct 19, 2018

fabien0102 commented Oct 23, 2018 • edited Loading

TejasQ commented Oct 23, 2018 • edited Loading

fabien0102 commented Oct 23, 2018 • edited Loading

peterszerzo commented Oct 23, 2018

fabien0102 commented Oct 23, 2018

TejasQ commented Oct 23, 2018

peterszerzo commented Oct 25, 2018

TejasQ commented Oct 30, 2018

peterszerzo commented Oct 30, 2018

fabien0102 commented Oct 30, 2018

Feature: Add support for validators that are promise-based or throw errors #72

Feature: Add support for validators that are promise-based or throw errors #72

fabien0102 Oct 19, 2018 •

edited

Loading

TejasQ Oct 19, 2018 •

edited

Loading

TejasQ Oct 19, 2018 •

edited

Loading

fabien0102 commented Oct 23, 2018 •

edited

Loading

TejasQ commented Oct 23, 2018 •

edited

Loading

fabien0102 commented Oct 23, 2018 •

edited

Loading