Modular orchestrator

[astorije]

⚠️ READY TO BE REVIEWED BUT NO TESTS YET ⚠️

One of the ideas here is that app.js should get as little intelligence as possible.

The result is an Orchestrator that controls the process of publication. This class manipulates a frozen (immutable) state, RequestState. Basically, everything the Orchestrator does is manipulate a state that is given as an argument of the computation so that the outer state is never modified as a side effect. That way, we avoid unexpected changes.

To help with this and abstracting, the code makes use of higher-order functions (functions given as arguments or returned by other functions). This separates concerns because the code for a function makes little assumption on what an argument is supposed to do in addition to its type (for example, if as an argument I give a function f: Int => Promise.<Int>, I don't really care what f is doing, I only care about what I feed it with and what I get in return). I'll keep improving the code to that extent to favor maintainability.

In the end, the main entry point is a recursive function that recurses on a function producing a promise until a predicate is reached. This will probably be hidden under the hood in a near future, but at least in the present state it allows us to do something (whatever it is, since it's an higher-order function) every time the request state is changed (that is, every time a step in the process makes it change). At the time of writing this, I am just displaying the new state on the console, but it is also a good place to consider persistence support.

[tripu]

Good luck with this one, @astorije… ;) Feel free to assign it to me when you're ready.

[astorije]

Cancelling Travis CI as style checker won't pass...

[astorije]

Pfiou, I just sucessfully rebased my 41 commits, that was something :-)

@tripu, I think I am more or less done here!
Before giving the go, there is one thing I'd like to confirm with you: this refactoring will help dealing with storing things. While developing, I was just console.logging what I was getting as an output, but I figured I might have lost some of your persistence-related code. After checking though, the original app.js just printed something in the console. In my understanding, the only thing that appears on the storage layer is appending to a log in the History. Could you confirm this? We can have a quick chat if this doesn't make sense.

If the answer to the previous question is no, then you can start reviewing. Meanwhile, I will provide tests now that this architecture offers it \o/ And by the time this PR is fully reviewed, hopefully it's gonna be fully tested too!

I have put a lot of love in this, I hope you'll like it :-)

[tripu]

@astorije, this LGTM!

I see this build failed, but only because code coverage dropped a little bit. No big deal now.

I think you are right in that the only “output” here was done through the module History. So if I understood the logic in your comment above, that means we're OK to continue (read: merge), right?

I've tried to follow your commits and the diff, but yeah, it's difficult :¬/ For what I can see, the core logic and the flow of steps and checks is preserved (and obviously tests pass), so I won't try to dig very deep.

About tests: I'd say we merge this right now, as is; and work on tests in a separate PR. I fear that if you, @plehegar or I tackle tests for this in this same branch and without merging to master first, the snowball is going to keep of growing. I predict new rebases and an even messier diff to review… Maybe I didn't understand you, and that is what you were suggesting anyway?

Please merge yourself (or grab @deniak if you want a third opinion; but I say go).

Good job :¬)

[astorije]

I see this build failed, but only because code coverage dropped a little bit. No big deal now.

98% is still a decent number :P
And the funny part is that when I'll start adding tests, the coverage will decrease as the new files in lib/ are not parsed yet (click on All).

I think you are right in that the only “output” here was done through the module History.

Okay, I have some ideas to improve this then :-)

So if I understood the logic in your comment above, that means we're OK to continue (read: merge), right?

Yes.

I've tried to follow your commits and the diff, but yeah, it's difficult :¬/ For what I can see, the core logic and the flow of steps and checks is preserved (and obviously tests pass), so I won't try to dig very deep.

Indeed, I essentially just made things modular and scalable. My worry was that I might have lost some existing behavior, mostly persistence and side-effects (since most of what's added in lib/ is side-effect-free).

About tests: I'd say we merge this right now, as is; and work on tests in a separate PR. I fear that if you, @plehegar or I tackle tests for this in this same branch and without merging to master first, the snowball is going to keep of growing. I predict new rebases and an even messier diff to review… Maybe I didn't understand you, and that is what you were suggesting anyway?

Well, I would never suggest merging without tests or biking without a helmet :D
But I agree that this PR is too big to be postponed further. A good outcome is that we should be able to work on the main loop in small bits from now on.
So okay to merge without tests, but if you're okay with that, let's leave it a couple of weeks before the next release to give some time to tests.

Please merge yourself (or grab @deniak if you want a third opinion; but I say go).

Here is my proposal: I'll merge only tomorrow. If @deniak, @darobin, @tabatkins (thanks for your input!) or @dontcallmedom has a free eye to spare on this (partially, even) by then, that's cool. Otherwise, no worries.
Meanwhile, I'll see if I can add a test or too and/or see if something needs fixing. I'll merge afterwards.

Good job :¬)

Thanks :-)

[dontcallmedom]

this is brittle (strings redefined across modules); I would make it a "static" property of RequestState. (i.e. add RequestState.started = "started", etc in lib/request-state.js)

[astorije]

Yep, I am planning to consolidate this very soon. Thanks!

[astorije]

See #148

[darobin]

I haven't looked very closely, but it looks like good stuff and I'm all for fast-merging of refactors that pass their tests. Otherwise things get really painful.

[astorije]

Huge thanks to @dontcallmedom for reviewing this. Everything you highlighted is very valid and I'll make sure to go through all of them, whether it's prior to or after merging.

@tripu, most comments will be addressed after merging I think, but there are a couple of things I'd like to change before merging, so definitely let me merge if you don't mind (or wait to merge yourself) :-)

[astorije]

@darobin thanks again for the input. Although I can entirely break Echidna and pass the tests with my changes, you know :-)

I'll process comments of both of you tomorrow, fixing right away what I can fix and opening issues/preparing PRs for the rest. I am very glad overall that you all like it! Thanks ❤️

[astorije]

Actually, after re-reading all comments, I am going to merge this now, open issues to document what was said and send PRs for these (sooner or later depending on the issue).

[tripu]

OK to remove the branch?