Example usage of new internal utils api #17

michaloo · 2017-02-20T14:49:32Z

I'm creating this PR to get an agreement on new API and context dependency container.

michaloo · 2017-02-21T15:01:51Z

Iterate on the ship's patterns and foundation

sbellity · 2017-02-20T16:12:31Z

docs/examples/context_definition.js

+  },
+
+  // superset of Hull API - helpers/utils:
+  agent: {


I think all those methods should be standard methods in hull client

Moved to standard client.

sbellity · 2017-02-20T16:20:07Z

docs/examples/context_definition.js

+    del: () => {},
+  },
+
+  // OPTIONAL - method to queue jobs to internal queue


I think this method name should be a verb. either enqueue or perform_async / perform_later. We have also see that some jobs need to be scheduled / performed in the future. You can also have a look at Sidekiq's API which works quite well for us already ;)

https://github.com/mperham/sidekiq/wiki

Changed to enqueue.

sbellity · 2017-02-20T16:21:59Z

docs/examples/context_definition.js

+  // yet may be adjusted freely by ship developer
+  service: {
+    client: {
+      get: () => {},


I guess actions performed on the service by the ship should be encapsulated in the service agent. I think we should keep this structure as simple as possible. No need to expose the client and agent separately here.

We can simplify that for the default architecture. The structure here is elastic and allows connector developer to pick best solution.

…fhandler handlers

…utilities

sbellity · 2017-02-24T16:43:44Z

src/helpers/filter-user-segments.js

+ * @param  {Object} user Hull user object
+ * @return {Boolean}
+ */
+export default function filterUserSegments(ctx, user) {


@michaloo as discussed earlier let's make the path configurable, and / or allow to pass a list of segment Ids also

sbellity · 2017-02-24T16:45:47Z

src/helpers/request-extract.js

+  if (segment) {
+    search.segment_id = segment.id;
+  }
+  const url = URI(`https://${hostname}`)


I would pass the hostname explicitly here. Not use the ctx directly.
hostname is bot available in a worker context, right ?

All helpers functions needs adjustments, I've bound them to the context for the simplicity and consistency of the signature, but I agree it's too heavy dependency for most of them. I will rework them.

sbellity · 2017-02-24T16:50:27Z

src/utils/action-handler.js

+  const router = Router();
+  router.use(requireHullMiddleware);
+  router.post("/", (req, res, next) => {
+    return Promise.resolve(handler(req.hull, _.pick(req, ["body", "query"]))).then(next, next);


I would directly pass the whole req object to the handler here. @michaloo do you see any reason not to do so ?

I found it quite confusing if I have fn(req.hull, req) as arguments, since I can get first from the second one. What would you tell about:

handler(req.hull, _.omit(req, ["hull"]))?

sbellity · 2017-02-24T16:52:21Z

src/utils/response-middleware.js

+ * @param  {Object}   res
+ * @param  {Function} next
+ */
+export default function segmentsMiddleware(result, req, res, next) {


oops wrong function name ?

sbellity · 2017-02-24T16:53:10Z

src/utils/segments-middleware.js

@@ -0,0 +1,37 @@
+import _ from "lodash";
+
+const fieldPath = "ship.private_settings.synchronized_segments";


we can use synchronized_segments as a naming convention and default value here but let's make this configurable.

unity · 2017-02-24T15:56:44Z

docs/CONNECTOR.md

+```js
+import Hull from "hull";
+
+const app = new Hull.App({ port, hostSecret, service });


we don't want to take over the app. We want to be an enhancement to an express app.
for 2 reasons

1 - it's less scary. People know how express works, and they understand it.
2 - sometimes (such as Slack bot or when integrating in your own stack) you just can't create a new app, you have to enhance an existing one in a specific way. Look at https://github.com/hull-ships/hull-slack/blob/develop/server/server.js

@unity just to sum up our discussion here - I all agree with your arguments, I find the express based architecture a good choice too.

I will just tell you that all that parts going far from plain express app, are caused by slightly different approach which I had in mind. After discussions with you and @sbellity trying to bring together your perspectives I came to a conclusion that the express application is only some part of our ship application, btw. the part which could be replaced or changed in some point in the future (for other http framework as koa or dropped for other solution).

But after seeing all the discussion here, I clearly see that you are all for having the express as a first citizen of the whole architecture. So this is my proposition which should reconcile everybody here :)

Let's make the sacred const app = express(); line being called within the ship

Let's make the current Hull.App a class returning a set of middlewares, routers and commands (so a method which takes the app and mutates it, applying some custom logic) - my perspective here is if I were the external ship developer I would find the app = express approach most comfortable for the reason you've brought here up. Yet observing the first actual developers doing their first ships I see that they are much much more focused on the business logic, the parts related to the 3rd party API that to the ships internals or infrastructure parts. I would say that the mind flow of a developer could be totally opposite to the natural flow of the application. That's why I will suggest having a "super command" method where you can start very easily having all the basics set up on our current convention (like manifest, README, templates), then you can turn it off and switch to more low level elements.

unity · 2017-02-24T15:57:41Z

docs/CONNECTOR.md

+
+// ... application configuration
+
+app.start();


Same thing here.

I don't know what app is but I know it's not express, and start() doesnt let me decide on the port in the place I'm used to. I don't know what routes are available, exposed and configured. Black box

As a regular Node developer knows app to be an express instance. Here it's app but it's something different.

unity · 2017-02-24T16:03:31Z

docs/CONNECTOR.md

+```js
+const app = new Hull.App({ port, hostSecret, service });
+
+const server = app.server();


This entertains the confusion even more.
We are using app (which is still not an express app) and calling server to create an express app...
And then we're not calling it app but server, and we're not setting it up.

The reason we want to set it up:

we want control over the port

we might want to add middlewares before and after hull does it's job

we want to be able to set the engines and static assets. Hull shouldn't force and own this. it's "good" boilerplate.

we can't setup an express router here unfortunately since routers can't have engines. router.set isn't supported

we might want to add middlewares before and after hull does it's job

This is still the hardest point of the architecture. Before we had the handlers/routers a blackboxes with all middlewares being applied (https://github.com/hull/hull-node/blob/master/src/notif-handler.js#L173), without the option to inject some custom logic between selected middlewares. Right now, they are split into a app level and router level, this allows for some degree of customization, but still not perfect.

I will come up with some new examples here too.

unity · 2017-02-24T16:06:30Z

docs/CONNECTOR.md

+const worker = app.worker();
+
+worker.attach({
+  customJob: (req) => { process }


attach isn't very clear. should add docs on how it works

I agree that attach might not be the right verb here.
Why not use ?

No problem, I will switch the naming here.

unity · 2017-02-24T16:14:35Z

docs/CONTEXT.md

+[Hull API client](../README.md) initialized to work with current organization
+
+#### ship
+Ship object with manifest information and `private_settings`


needs some example data

unity · 2017-02-24T16:38:47Z

docs/MIDDLEWARES.md

+```js
+import Hull from "hull";
+
+app.use(Hull.Middleware({ hostSecret: "supersecret" }));


here, what is app. is it an express app or a hull app?
It seems here it's express - example of confusion

I know it's my paragraph here ;P

unity · 2017-02-24T16:41:23Z

docs/MIDDLEWARES.md

+- If the configuration or the secret is invalid, an error will be thrown that you can catch using express error handlers.
+
+```js
+app.use(function(req, res, next){


This one is a very important one.

Developer MUST be allowed to define his own middleware in the middle of the stack to define how the Token will be retrieved and put in the right place (req.hull.token) or the right keys (req.hull.config)

unity · 2017-02-24T16:42:13Z

docs/MIDDLEWARES.md

+
+app.use((req, res) => { res.json({ message: "thanks" }); });
+
+app.use(function(err, res, req, next){


same here. this MUST be something the developer can do. a generic error catching middleware to avoid crashes where he can decide what to return

Cant' take this away from them

unity · 2017-02-24T16:42:46Z

docs/MIDDLEWARES.md

+
+## serviceMiddleware
+This is a middleware which helps to inject the custom ship modules to the request object.
+Thanks to that, all the custom functions and classes are automatically bind to the context object:


unity · 2017-02-24T16:44:45Z

docs/MIDDLEWARES.md

+app.post("/get-segments", (req, res) => {
+  const { service } = req.hull;
+
+  service.getSegmentIds({ limit: 500 })


why do this. I feel

import getSegmentIds from "get-segment-ids" ... getSegmentIds({limit: 500, context: req.hull })

is just as simple and has even less side-effects.
What's the benefit here?

…ress app

…utilities

Example usage of new internal utils api

fd8731a

michaloo requested review from sbellity and unity February 20, 2017 14:49

michaloo added 15 commits February 20, 2017 17:00

Adds docs readme to document the ideas/problems

8649cea

Updates readme

97fc2c6

Updates readme

f03969c

Iterate over the API

5bd1222

Major refactor to match the new API

f24efbd

Basic lint

04e10ca

Fix build task

c9328c0

Add service middleware

4bd6c11

Adds more docs and simplify the API

4885eeb

Organize the docs

d78bf30

Updates the main docs

800bc1a

Adds more docs and remove sudo param

eb02258

Improve docs

c6ba1ec

Update docs and improve functions consistency

7ce28f9

Major lint

81aa11b

sbellity reviewed Feb 21, 2017

View reviewed changes

michaloo added 10 commits February 22, 2017 10:50

Moved the files outside ship directory, created HullApp

69ba3d1

Revert deleted code

18a6b5c

Initial tests

d0051a7

Rename agent to helpers

3ce745f

Adds example of process splitting

973b336

Enhance docs and memory queue

d7ae359

Enhance defaults

58264a4

Migrate tests from hull-ship-base

2ef88a7

Switch batcher from req to ctx dependency

685688d

Adds token middleware into the HullApp, changes the signature of noti…

3931d38

…fhandler handlers

michaloo added 20 commits February 23, 2017 14:20

Update docs

2dd5b20

Update docs

32d2c6a

Update docs

6dcd701

Update docs

f104dfb

Update examples

5e6a5cb

Temporary - add postinstall

2debf31

Temporary - add postinstall

8fbc56c

Temporary - add postinstall

7b7eb76

Temporary - add postinstall

8c75af7

Update docs

a811a7a

Temporary - add postinstall

b66f65e

Make notify middleware more loose

fdfd8db

Make the token middleware catch the oauth token

6bacb2b

Prepare code of different passport strategies signatures

bd45ad0

Prepare code of different passport strategies signatures

7a7b23f

Prepare code of different passport strategies signatures

536969f

Adds server for existing express and middleware method

21c6e51

Fix docs and examples

87cf142

Remove temporary build scripts and mark new version

7192f20

Merge branch 'master' of github.com:hull/hull-node into feature/ship-…

7874962

…utilities

sbellity reviewed Feb 24, 2017

View reviewed changes

unity reviewed Feb 24, 2017

View reviewed changes

michaloo added 5 commits February 27, 2017 17:29

Rename Hull.App to Hull.Connector and adjust the API to alter the exp…

f460ca1

…ress app

Update examples and fix middleware calls

fec4e08

Fix exit handler

cd41111

Fix examples and cache testing

5d8bb89

Merge branch 'master' of github.com:hull/hull-node into feature/ship-…

fa12849

…utilities

michaloo changed the base branch from master to release/0.11 February 28, 2017 13:58

michaloo merged commit 455a79d into release/0.11 Feb 28, 2017

michaloo deleted the feature/ship-utilities branch February 28, 2017 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Example usage of new internal utils api #17

Example usage of new internal utils api #17

michaloo commented Feb 20, 2017

michaloo commented Feb 21, 2017

sbellity Feb 20, 2017

michaloo Feb 22, 2017

sbellity Feb 20, 2017

michaloo Feb 22, 2017

sbellity Feb 20, 2017

michaloo Feb 22, 2017

sbellity Feb 24, 2017

sbellity Feb 24, 2017

michaloo Feb 27, 2017

sbellity Feb 24, 2017

michaloo Feb 27, 2017

sbellity Feb 24, 2017

sbellity Feb 24, 2017

unity Feb 24, 2017

michaloo Feb 27, 2017

unity Feb 24, 2017

unity Feb 24, 2017

michaloo Feb 27, 2017

unity Feb 24, 2017

sbellity Feb 24, 2017

michaloo Feb 27, 2017

unity Feb 24, 2017

unity Feb 24, 2017

unity Feb 24, 2017

unity Feb 24, 2017

unity Feb 24, 2017

unity Feb 24, 2017

unity Feb 24, 2017

		@@ -0,0 +1,37 @@
		import _ from "lodash";

		const fieldPath = "ship.private_settings.synchronized_segments";


		app.use((req, res) => { res.json({ message: "thanks" }); });

		app.use(function(err, res, req, next){

Example usage of new internal utils api #17

Example usage of new internal utils api #17

Conversation

michaloo commented Feb 20, 2017

michaloo commented Feb 21, 2017

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

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

Choose a reason for hiding this comment

This one is a very important one.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment