Expand fs-ts in Prism's router #402

XVincentX · 2019-07-06T13:10:30Z

The following PR extends the usage of fp-ts to the routing package as well by basically making sure it does not throw exceptions anymore, but rather use the Either object to express an error object.

With this — the router and the mocker finally compose because the type match (they both return an Either<Error, T>.

Extend the Either usage to the router was indeed the easy part: https://github.com/stoplightio/prism/pull/402/files#diff-f9a10b37616fb5669ecd5218fc8535c9L16

The problem started when I started to integrate and try to compose the new function in the mega-file-to-split:

The whole flow is sync apart from the edge case when we need to employ the forwarder, and this requires an additional abstraction layer
https://github.com/stoplightio/prism/pull/402/files#diff-47c5dc2d65fd624c869f5f08d0cfb56aR45
What's really preventing from having a clean and functional flow is the validation process that's basically creating an empty array, giving it to the mocker and expecting to receive a filled array. This forces me to keep some stuff here and some stuff there; if the mocker could just return the validations, that'd improve the code a lot.
In order to keep the API compatible with what we have, I have to do some wrapping I'd like to avoid
https://github.com/stoplightio/prism/pull/402/files#diff-47c5dc2d65fd624c869f5f08d0cfb56aR98

That said, the funny thing is that, although this Pull Request is meant to be an improvement, you can argue that the code is effectively uglier than it is. (Well I do not think it is, but you mileage may vary)

The good news though is that — I'm not sure if you remember, we were discussing on how to refactor this part and nobody (me included) really came up with good ideas.

By trying to extend the functional parts to the router — I now know exactly what needs to be done and how to move forward. This is freaking awesome, to be honest.

karol-maciaszek

I can admit that it takes some time to see the goal (and what those tons of helper functions do).

I've put some questions and comments.

One general note: the code looks complicated because it is long and deeply nested. How about enclosing parts into descriptive functions, so a human can see the overall flow and can dig into details if needed.

Sth like:

router.route()
.fold(modyfingSomehowLeftAndRight)
.chain(doingSthUnderstandableToLeftAndRight)
...

Second thing: Either.getValidation pattern seems to fit better here?

karol-maciaszek · 2019-07-08T13:36:58Z

packages/core/src/factory.ts

+                const { message, name, status } = error as ProblemJsonError;
+                // otherwise let's just stack it on the inputValidations
+                // when someone simply wants to hit an URL, don't block them
+                inputValidations.push({


Pushing things to array which is defined outside function chains feels not right. How about using validations? (https://dev.to/gcanti/getting-started-with-fp-ts-either-vs-validation-5eja - Validation section in the middle)

Yes! That's exactly what I wanted to do. However I'm proceeding with baby steps here as — you've probably noticed — it's hard to digest all in one place; moreover we're close to the release and I didn't want to change that many things at the time.

…but yes it's exactly the direction I want to take!

karol-maciaszek · 2019-07-08T13:37:39Z

packages/core/src/factory.ts

              },
-              defaultComponents.mocker,
+              value => right2v(value),


I had a problem finding documentation for right2v. Isn't it deprecated?

It is deprecated in 2.x — Prism is currently on 1.9. I'll migrate it soon though.

packages/core/src/factory.ts

XVincentX · 2019-07-08T14:10:22Z

One general note: the code looks complicated because it is long and deeply nested. How about enclosing parts into descriptive functions, so a human can see the overall flow and can dig into details if needed.

Yeah I will probably try again — I gave up because every function was taking something like 3000 parameters complicating the thing anyway. I'm willing to give it another try though.

Thanks for the review!

chris-miaskowski · 2019-07-09T13:57:01Z

packages/http-server/src/__tests__/server.oas.spec.ts

-
-    expect(response.statusCode).toBe(200);
-  });
+  test.todo(


Such a decent test :( why remove it?

It was skipped — I'm fine putting it back but it's not getting executed anyway. I'd rather see a todo that says "Hey write this" instead of a skip that you do not easily see

chris-miaskowski · 2019-07-09T13:59:22Z

packages/http/src/__tests__/http-prism-instance.spec.ts

-          }),
-        ).rejects.toThrowError(ProblemJsonError.fromTemplate(UNPROCESSABLE_ENTITY));
-      });
+      test.todo('with invalid body returns validation errors');


🔥 🔥 🔥 You're removing my lovely tests :( 🔥

It was skipped — I'm fine putting it back but it's not getting executed anyway. I'd rather see a todo that says "Hey write this" instead of a skip that you do not easily see

chris-miaskowski · 2019-07-09T14:01:15Z

packages/core/src/factory.ts

-                resource,
-                input: { validations: { input: inputValidations }, data: input },
-                config: configObj,
+                return right2v<Error, Resource | undefined>(undefined);


right2v, whoa what a lovely interface :P I have a suggestion
right2lolz because why not? Ok enough sarcazm for today

It's actually a good point — it has been fixed in 2.x but I didn't want to deal with breaking changes right now.

We'll get less PR from random people. Entry threshold is set high ;)

chris-miaskowski

Ok, so all the "code style" related comments aside I think it's a very decent change and good step forward.
But I take your word on the Lunch & Learns @XVincentX ! :P

accepted by Chris

philsturgeon · 2019-07-09T16:13:25Z

We should keep skipped tests in place and try to mark them. Please add notes explaining why they’re skipped and make stories to finish implementing them. It’s tech debt we need to resolve. If we just delete them then we’re ignoring technical debt and that’s not ideal!

…

-- Phil Sturgeon @philsturgeon

On Jul 9, 2019, at 16:07, Vincenzo Chianese ***@***.***> wrote: Merged #402 into master. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

XVincentX · 2019-07-09T16:51:44Z

@philsturgeon

Just to clarify — the tests haven't been removed; I've just moved them from test.skip to ´test.todo´. I also left the comments.

The current implementation there wasn't useful anyway and just gave the impression that the test was done. Now we can clearly see that the test is not done, and we'll fix that in the future.

philsturgeon · 2019-07-09T23:43:28Z

Sounds awesome, good work!

…

-- Phil Sturgeon @philsturgeon

On Jul 9, 2019, at 18:51, Vincenzo Chianese ***@***.***> wrote: @philsturgeon Just to clarify — the tests haven't been removed; I've just moved them from test.skip to ´test.todo´. I also left the comments. The current implementation there wasn't useful anyway and just gave the impression that the test was done. Now we can clearly see that the test is not done, and we'll fix that in the future. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

XVincentX added 6 commits July 6, 2019 12:45

refactor: router should not throw anymore

4183898

refactor: router returns an either

6f1ae0f

refactor: fp-ts in router

42e4d22

test: refactor router tests

e45b0a8

refactor: orElse

7ca5429

test: fix

7b72eaf

XVincentX force-pushed the feat/fp-ts-router branch 2 times, most recently from 1e0ff9f to 9f3aced Compare July 7, 2019 07:28

XVincentX marked this pull request as ready for review July 7, 2019 07:30

refactor: use foldTaskEither

5742996

XVincentX force-pushed the feat/fp-ts-router branch from 9f3aced to 5742996 Compare July 7, 2019 07:31

XVincentX added 2 commits July 7, 2019 09:33

test: todo

de8ba3a

refactor: fromEither is not required

7c802d9

XVincentX force-pushed the feat/fp-ts-router branch from 95544d6 to 7c802d9 Compare July 7, 2019 09:23

refactor: get rid of fromEither

347b589

XVincentX requested review from chris-miaskowski, philsturgeon and lag-of-death July 7, 2019 10:41

Merge branch 'master' into feat/fp-ts-router

4accf3b

karol-maciaszek previously requested changes Jul 8, 2019

View reviewed changes

XVincentX added 2 commits July 8, 2019 16:03

refactor: keep the async keyword

7b728d2

Merge branch 'master' into feat/fp-ts-router

22600a5

refactor: put a different function

d972f3a

XVincentX self-assigned this Jul 9, 2019

chris-miaskowski reviewed Jul 9, 2019

View reviewed changes

chris-miaskowski approved these changes Jul 9, 2019

View reviewed changes

XVincentX merged commit 555d3f0 into master Jul 9, 2019

XVincentX deleted the feat/fp-ts-router branch July 9, 2019 14:07

Expand fs-ts in Prism's router #402

Expand fs-ts in Prism's router #402

XVincentX commented Jul 6, 2019 •

edited

karol-maciaszek left a comment

karol-maciaszek Jul 8, 2019

XVincentX Jul 8, 2019

karol-maciaszek Jul 8, 2019

XVincentX Jul 8, 2019

XVincentX commented Jul 8, 2019

chris-miaskowski Jul 9, 2019

XVincentX Jul 9, 2019

chris-miaskowski Jul 9, 2019

XVincentX Jul 9, 2019

chris-miaskowski Jul 9, 2019

XVincentX Jul 9, 2019

karol-maciaszek Jul 9, 2019

chris-miaskowski left a comment

philsturgeon commented Jul 9, 2019 via email

XVincentX commented Jul 9, 2019

philsturgeon commented Jul 9, 2019 via email

Expand fs-ts in Prism's router #402

Expand fs-ts in Prism's router #402

Conversation

XVincentX commented Jul 6, 2019 • edited

karol-maciaszek left a 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

XVincentX commented Jul 8, 2019

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

chris-miaskowski left a comment

Choose a reason for hiding this comment

philsturgeon commented Jul 9, 2019 via email

XVincentX commented Jul 9, 2019

philsturgeon commented Jul 9, 2019 via email

XVincentX commented Jul 6, 2019 •

edited