Refactors PushController and FilesController to support multiple apps

[flovilmart]

effort to decouple the heavy lifting of #263

[facebook-github-bot]

@flovilmart updated the pull request.

[nlutsenko]

There are few things worth discussing here - the flow that we are trying to get into is that FIlesController/PushController/etc don't actually know about HTTP layer at all, they contain a business logic for retrieving files or sending push notifications.

Tight coupling of request/response/next with them is an interim state, that I am trying to move us away from. The goal here is that we will be able to unit test everything absolutely without relying on integration or e2e test as a first resort, but have unit tests for the whole server.

Another important piece - Routers actually follow the same architecture. A single class per endpoint type (say classes or files), which encapsulates logic for handling HTTP layer, and can talk to Controllers to figure out the logic.

All of the above (Controllers/Adapters/Routers) are instances of different classes, so we can reuse logic with subclassing/etc, as well as the entire thing becomes purely functional, since we have a single consistent state that is created at the point of construction of the server.

I might be wrong in my assumptions here, but this architecture proved to be very helpful in terms of testability, modularization and stability. It's closely modeled after what me and SDK team designed to power all of our SDKs.

So... Let's discuss it... I am looking for your feedback on this, so we can move together faster.
Which part of this is great, which one is not? Which part would you do differently?

[flovilmart]

There are few things worth discussing here - the flow that we are trying to get into is that FIlesController/PushController/etc don't actually know about HTTP layer at all, they contain a business logic for retrieving files or sending push notifications.

So with the current implementation we're completely off that target.
As you can see, the PushController holds the expressRouter, as well as request processing.

What we should have is

class PushRouter extends PromiseRouter {
   constructor() {
     this.route("GET", "/something", this.doGet);
   }   

  doGet(req) {
     const interestingThing = req.body. interestingThing;
     const interestingOtherThing  = req.body. interestingOtherThing;
     if (! interestingThing) { 
        throw new Parse.Error(INVALID_JSON, 'missing interesting thing');
     } 
     // we ensure no shared state, nor leaking 
     // across multiple apps or so.
     // The push controller is testable by itself
     // The functions are testable too with 'fake' req.
     const controller = req.config.pushController;
     return controller.doSomething(interestingThing, interestingOtherThing).then( (result) => {
        /// format the result here, based on the request maybe?
        return result;
     });
   }
}

[nlutsenko]

Yup, that example above is perfect! I couldn't write better in my 3 week-old knowledge of javascript. :)

There is a lot of places where we are off target, but it's ok.
We iterate over it, fix bugs as we go, as well as write tests where we see not tested pieces.
The most important thing about any large rearchitecture like this that I am following is that it shouldn't slow down anyone from contributing.

[flovilmart]

I'm revamping the original Classes, User, Installation etc... with that pattern, I'll start applying it across the board :)

[facebook-github-bot]

@flovilmart updated the pull request.

[nlutsenko]

Looks amazing! Should I just merge it now or there is more things coming in this one?

[flovilmart]

I'm doing all routers that can be, Functions, Schemas as well
I'ill leave Push and Files as they are as they need to be completely split into Router/Controller

[nlutsenko]

Lovely, sounds good. Ping me when you need a review on this.

FilesController is quite tricky, there is direct dependency on non-promise based router, so there is more changes needed for it, I might as well try to tackle it tonight.

[flovilmart]

Yeah, and PushController too :) That,S why I don,t want to touch them yet :)

[facebook-github-bot]

@flovilmart updated the pull request.

[flovilmart]

And you got it! :) Good luck! The refactoring is mostly external, placed some todos here and there for some of the internals to be updated at a later date.

[flovilmart]

@nlutsenko and if you're curious, have a look there: https://github.com/flovilmart/__AppStack__/

[nlutsenko]

And looks great, merging in a moment...
Re: AppStack - not sure I got the overall idea, but the implementation looks very very familiar 😁

[flovilmart]

@nlutsenko, started that in November last year in the eventuality I had to move servers away from Parse