question: async function support in arc.http

brianleroux commented 6 years ago

per https://github.com/arc-repos/arc-example-login-flow/issues/3

not sure how to impl and would love feedback @mikemaccana !

maybe something like

let arc = require('@architect/functions')

// normal fun
function home(req, res) {
  res({html: 'hi from home'})
}

// async fun
// expect the second param to be 'next' if the function is an instance of AsyncFunction ???
async function log(req, next) {
  console.log('idk?')
  next()
}

exports.handler = arc.http(log, home)

austinkelleher commented 6 years ago

Should we allow using await next()? This is the pattern for Koa middleware.

brianleroux commented 6 years ago

oh I like it! but should we support the whole idea of context then too ?

austinkelleher commented 6 years ago

Good question. I think we should so that we can access different information in middleware. It would be particularly interesting if the API was consistent such that we could use async Koa middleware inside of arc. Not sure how realistic that is, but would be pretty awesome.

mikemaccana commented 5 years ago

Express 5 supposedly supports async/await, but just in terms of having async route functions - you can use await inside them, but you're still expected to call response(). See https://github.com/pillarjs/router (which Express 5 uses) for details. This isn't ideal - functions should return values, like the way Arc does already when you're not using middleware.

Koa middleware uses await next() - this article provides a good overview but I think this is unnecessarily complicated too. Koa isn't that popular that there's a huge ecosystem of middleware to take advantage of yet.

A series of middleware tasks is - a series of middleware tasks. A series of things is best represented as an Array or arguments- this allows us to get rid of next() entirely:

I've removed the demo code, here's a working implementation!

mikemaccana commented 5 years ago

Updated!!!

This has a few small tweaks - see the docs below.

Working async middleware!

Docs

Middleware (arc.middleware)

You can combine multiple operations in a single route using the middleware API. This is similar to middleware processing in other node frameworks, but much simpler. Just run arc.middleware() specifying each piece of middleware as arguments. Requests will be run through each middleware in the order they're given to arc.middleware() with the following rules:

If the middleware doesn't return anything, the request will be passed to the next middleware in the queue

If the middleware returns a modified request, the modified request will be passed to the next middleware

If the middleware returns a response, this will end processing and send the response back.

Here's an example in which we'll register arcGeoIP.getCountryCodeMiddleware, requiresLogin, and showDashboard to run in series.

arcGeoIP.getCountryCodeMiddleware adds countryCode to our request

requiresLogin will return a redirect response if we're not logged in (ending processing) or continue if we are logged in

showDashboard will show a dashboard for users, since we know they're logged in.

let arc = require("@architect/functions"),
    respond = require("@architect/shared/respond"),
    arcGeoIP = require("@architect/shared/arc-middleware-geoip")("US"),
    [print, debug, log, warn, alert] = require(`boring-utils`);

async function requiresLogin(request) {
    let state = await arc.http.session.read(request);

    log(`State is: ${print(state)}`);

    if (!state.isLoggedIn) {
        log(`Attempt to access dashboard without logging in!`);
        return respond.redirect(`/login`);
    }
    log(`We're logged in`);
    // return nothing, so middleware continues
}

// No async-using middleware (ie, a separate requiresLogin) since that's not done in arc yet https://github.com/arc-repos/architect/issues/193
async function showDashboard(request) {
    log(`Showing dashboard`);

    let dashboardPage = `
    <body>
        <h1>Dashboard</h1>
        <p>oh hai you are logged in from ${request.countryCode}! <a href="/logout">logout</a><p>
    </body>`;
    return respond.makeResponse(dashboardPage);
}

exports.handler = arc.middleware(arcGeoIP.getCountryCodeMiddleware, requiresLogin, showDashboard);

brianleroux commented 5 years ago

oh dude! I like this a lot… hmmmmmmmmm… wonder if we could make arc.http smart enough to know you want this style vs the express-y style (alternatively we could deprec the expressy stuff and move this way)

mikemaccana commented 5 years ago

Thanks! 😃 My general feeling is to deprecate the express style in the next major release:

One, obvious way to do things is best (not just for users but for maintenance too). This is less complicated than the Express style due to not needing next() and also more consistent with regular await style routes. Ie, the whole "how do I do middleware?" becomes much simpler.
The existing Express-style implementation isn't as 'express-like' as one would think - we don't allow the request to be mutated between middleware. A different signature is a good way of indicating users should have different expectations.

mikemaccana commented 5 years ago

Just a quick note I might have something even better than this, heading to gym now but will post again in a few hours.

mikemaccana commented 5 years ago

OK done. Check the updated example above!

mikemaccana commented 5 years ago

Also I've called this arc.middleware because it's a little clearer (and also a good break from the older arc.http style).

brianleroux commented 5 years ago

love it! send a pr to github.com/arc-repos/arc-functions and we'll get that in!

mikemaccana commented 5 years ago

Rock and roll. Going to learn tape and send a PR and some tests shortly.

On Fri, 7 Dec 2018 at 4:35 pm, Brian LeRoux notifications@github.com wrote:

love it! send a pr to github.com/arc-repos/arc-functions and we'll get that in!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/arc-repos/architect/issues/193#issuecomment-445288819, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKiMhFeNoVGl9t_2B91khd4wBC0vvBhks5u2pi1gaJpZM4YW91a .

mikemaccana commented 5 years ago

I've done this in https://github.com/mikemaccana/arc-functions (see https://github.com/mikemaccana/arc-functions/commit/a383455c1d93faf271863377a4030a045b7d3408) but after npm i https://github.com/mikemaccana/arc-functions I can't actually invoke the new code for some reason.

Even completely removing .\node_modules\@architect\functions doesn't even complain in the sandbox when I use a route that runs arc = require("@architect/functions") - I just get a generic async error. Either something is throwing away all the errors or I'm not operating on the code I think I am. It feels like I'm going insane.

brianleroux commented 5 years ago

did you install into the function in src/http/ or in the root ?

mikemaccana commented 5 years ago

The file I modified in the module arc-functions was src/http - see https://github.com/mikemaccana/arc-functions/commit/a383455c1d93faf271863377a4030a045b7d3408#diff-b260296cc92b313a7417080879976bcb

I installed arc-functions module into my app via npm / github.

brianleroux commented 5 years ago

rather than huck out the old code which maybe has a dep could you amend to just add arc.middleware and leave the old stuff around (should be easier to reason about)

mikemaccana commented 5 years ago

Everything else was arc.http.foo so that seemed logical - is arc.http.foo going away?

Happy to, but I'm flying blind if I can't get any errors. It's entirely possible I'm fatigued right now (it's the end of the day here and it's been Rubik's cubes all day).

I'll revert to current upstream, add a console.log or throw an error and see if that works before going any further.

brianleroux commented 5 years ago

word! we're doing a big migration this morning so I'll HOPEFULLY be back to give this a go myself this afternoon… 🤞

mikemaccana commented 5 years ago

OK I'm not going nuts - there's some mystery mechanism loading @architect/functions when it's deleted from disk: https://stackoverflow.com/questions/53724176/how-do-i-know-where-is-module-being-loaded-from-in-node

MasterJames commented 5 years ago

It must be installed globally. Try npm uninstall with the -g flag

mikemaccana commented 5 years ago

Nah (good suggestion though) - the cause is the copying that happens when running the sandbox (see the SO post above) :

console.log(require.resolve("@architect/functions"));

C:\Users\mikem\OneDrive\Documents\serverless-commerce\serverless-commerce\src\http\get-dashboard\node_modules\@architect\functions\index.js

mikemaccana commented 5 years ago

So I just realised I misread someting earlier:

@brianleroux said:

did you install into the function in src/http/ or in the root ?

I thought he was asking about the file I modified in the functions module (which was also called /src/http/index.js coincidentally) 😂

In reality he meant I should npm i https://github.com/mikemaccana/arc-functions into src\http\get-dashboard (ie the route I was testing the middleware with) per https://arc.codes/guides/deps

mikemaccana commented 5 years ago

OK all good now, check out the PR at https://github.com/arc-repos/arc-functions/pull/14

Example above has been updated to use arc.middleware too.

brianleroux commented 5 years ago

nice, gotta update the changelogs with this next! ❤️

(also works for other function types like queues and events eh!)

mikemaccana commented 5 years ago

Cool, I'll update the docs too if you don't do it first!

On Sat, 15 Dec 2018 at 7:37 pm, Brian LeRoux notifications@github.com wrote:

Closed #193 https://github.com/arc-repos/architect/issues/193.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/arc-repos/architect/issues/193#event-2028592469, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKiMilcdVmUSW5vx4zuogh0Nq2V5rR2ks5u5U-LgaJpZM4YW91a .

mikemaccana commented 5 years ago

OK docs themselves now using arc 4, covering arc.middleware and the arc4 session API. Also docs site itself uses module.exports = arc.middleware(openMarkdown, notFound) which is a neat demo of the middleware API too! 😃

https://github.com/arc-repos/arc.codes/pull/54

architect / architect