An alternative to the "Functional River"

auth.js

// Easier to follow. Equally easy to test -- and with one fewer function in need of testing.

const Promise = require("bluebird");
const { hashStringAsync } = Promise.promisifyAll(require("./lib/crypto"));
const { logEventAsync } = Promise.promisifyAll(require("./lib/log"));
const { openAsync } = Promise.promisifyAll(require("./lib/db"));
const { TimeoutError, ValidationError, NotFoundError } = require("./errors");

const _openHandle = openAsync(); // FYI: Promises include memoization (caching) built into same API

module.exports = { auth };

/* auth is our main function */
async function auth({ username, password,
    _onTimeoutError = errorHandler,
    _onNotFoundError = errorHandler,
    _onValidationError = errorHandler,
}) {
    try {
        // No more reusing "password" as "hashed password".
        // Still starting the hash ASAP and not waiting for it.
        const hashedPasswordPromise = hashStringAsync(password);
        authValidate({ username, password });
        const users = await usersModel();
        const user = await users.findOneAsync({
            username,
            // I would have renamed "password" here; bad form to have a field named
            // password that's expecting a hash.
            password: await hashedPasswordPromise, // Deferred await to when we need it.
        });
        await userFound(user);
        return userInfo;
    } catch (e) {
        // Left this in because Bluebird's extension for matching errors is cute.
        Promise.reject(e)
            .catch(TimeoutError, _onTimeoutError)
            .catch(NotFoundError, _onNotFoundError)
            .catch(ValidationError, _onValidationError)
            .catch(errorHandler);
    }
}

function authValidate({ username, password }) {
    if (!username || username.length < 4) {
        throw new ValidationError("Invalid username. Required, 4 char minimum.");
    }
    if (!password || password.length < 4) {
        throw new ValidationError("Invalid password. Required, 4 char minimum.");
    }
}

function userFound(results) {
    if (results) return;
    throw new NotFoundError("No users matched. Login failed");
}

function usersModel() {
    return _openHandle.then(({ models }) => models.users);
}

function errorHandler(err) {
    console.error("Failed auth!", err);
}

Thanks for the reply!
I really appreciate you taking the time.

I'm not sure why os.userInfo is introduced. Did i miss some notes?

The Functional River pattern is dependent on a few design changes that wouldn't impact your code at all, yet would allow versatile consumption of your code.

In authValidate, adding a return { username, password } to the end doesn't harm your usage, while it allows my more FP-oriented approach as well.

Relatedly, userFound could return the user it's given w/o any negative impact to your current design.

I find it strange async/await shouldn't be used with return - for example you're not supposed to do this:

return await userFound(user);
// versus:
await userFound(user);
return user;

I know this isn't specific to your code. (And I do know the underlying reason is related to how call-site binding works with return.)

It's an awkward syntax design change that invalidates many published books/articles about how functions work ("return is the only way to return a value. There are no automatic returns like other language. etc."). It's a little ironic tc39 made this choice while being so averse to breaking JS for even the most ancient websites.

Eric Elliot wrote a great article that explains Pure functions in a way that details the value proposition - not just the academic definition.

The os.userInfo was my editor being overzealous. VS Code has an auto-import. I didn't notice it happen. Just a bug. Deleted. Thanks.

On return await userFound(user) -- the only reason that's discouraged is that it's redundant. You can just return userFound(user). If you return a Promise in an async function, it will be directly returned to the caller as a promise. Otherwise by calling await you're blocking until the promise resolves before you return. 99% of the time you actually don't want to block inside the function. What if the return value is intended for a Promise.all, and the intent was to complete your local processing in parallel with other work being done? (Edit: It doesn't matter much, thinking about it. It just generates a spurious extra promise wrapper, and may pause your processing by one "tick" of the Node event loop. But it's still unneeded/redundant.)

Having authValidate return{ username, password } doesn't actually help in the code above. That's why I deleted it. It's noise, and it implies that authValidate() might actually change username or password. Better to be explicit.

There's an entire dimension that I didn't cover, which is that passing around "all of the information you need" in the functional approach you're advocating is actually an anti-pattern. I'm all for functional flow for processing data; I love the idea of Observables. As I mentioned in the other (long) thread, I also use functional chaining when it makes sense. But it makes sense as long as the functional chain is transforming a single piece of data. Your example above is mixing data types, and it shows in how hard it is to understand why, for instance, authValidated is passing {username, password} to usersModel.

In your flow you have:

return Promise.resolve({username, password})
    .then(authValidated)
    .then(usersModel)
    .then(users => {
      return hashedPasswordUserQuery({username, password})
        .then(users.findOneAsync)
        .then(userFound)
    })

So it's not a single bit of data being transformed. Instead it's more of a hack: You're passing in {username, password}, and yes, authValidate returns that and passes it to usersModel (which confusingly doesn't take it as input), and then usersModel does something completely orthogonal and you revert to referencing the function parameters from the enclosing context {username, password}. At which point you've actually violated your functional river, by the way.

This is bad for more than aesthetic reasons. If authValidate actually needed to modify {username, password}, then this code would have a bug, and that's precisely because you're coercing the functional approach into an awkward construct. I made it explicitly not return a value that was going to be discarded.

Whereas if authValidate did need to modify {username, password}, simply changing my code to:

// With or without await, as appropriate.
{username, password} = await authValidate({username, password});

...then the pair would be updated by the result of the call, and would be bound in the continuation and available to later functions.

But as I'm sure you'll recognize, your example is actually simplified from the kinds of things that happen in the real world. In my other Gist:

https://gist.github.com/TimMensch/ce75d232096f2cfd623cf28fd1da160e

...I show a slight addition that would be much harder to do well in a "functional river."

Assume there's a cost to getting the models from _openHandle, and you want to only call it once to get both models. So you want to grab them early. Add in the "you might need to modify username or password" above, and you're passing {username, password, users, admin} through the promise chain (to properly handle the mutation of username and password).

See what's happening? You're manually binding local variables to each function. Add another variable, and suddenly you need to change all the functions. Or drop the destructuring so that the authValidate function doesn't need to know about the database interfaces.

If I were forced to use a pre-async/await JavaScript, I would have coded your example differently in fact. Probably something like:

return Promise.all([
    findUser({username,password}), // this would include ONLY the username,password part of the chain, keeping it pure
    usersModel()
])
    .then( ([userPass,users])=> users(userPass) )
    .then(userFound);

This creates two parallel streams, and each of the streams processes a single data stream, which are then combined by Promise.all. No awkward code pulling back from the enclosing scope. All the data flows through in a clean manner. All of the individual functions actually behave in a clean functional manner.

(Edited to fix syntax/semantics in the last example)

I agree the interfaces I came up with were a little contrived - it's a balance I tried to strike while still being somewhat copy-pastable.

Too many articles talk about Functional Purity using un-relatable or super granular examples: Math.max.

I wanted to show an applied & recognizable pattern to most (backend) devs. This necessarily means I have to make trade-offs: so it's both digestible, and not overly simplified.

Figuring out how to scope your closures & pass variables between steps is (to me) a separate qualitative discussion.
Differing priorities will dictate how this gets decided. In researching this area, I've been working on an (unpublished) article for about a year detailing how this plays out in Haskell, Erlang, Elm, and F#. (Each have varying ways to 'pass-through' values, or scope values using Monads, etc.) I believe this will address much of your misgivings.

---

I'm a little confused when I hear things like the "...functional approach you're advocating is actually an anti-pattern."

You're not the first person to tell me that - and I'm trying to understand where this value-judgement is coming from.

Before I continue, let me mention that the word 'Functional' gets thrown around to mean many different things. This is unfortunate. (And I know I'm not helping with a library called Functional Promises) - I don't want to get bogged down on whether I mean strictly Functional Programming ala Computer Science Purists or referring to Lambda calculus designs of math purists... This is a debate for another day. For now, let's unpack the "functional/anti-pattern" thing...

Anti-patterns in code are rarely absolutes. Everything is about trade-offs.

So, trying to figure out exactly you're talking about:

• If you were referring to my scoping decisions - I'm sure they could be improved. PRs welcome. 😺
• If however you are talking about function purity: parameters & return values... I must respectfully disagree. 😇

In Eric Elliot's article he alludes to the importance of return values (and that they ought to be useful) here:

It took me a long time to realize the benefit of this. Mostly because I was raised in an OOP Java/C# world: I used to think it was perfectly fine to return true for isValid() style functions. After many years, I see how brittle & opaque this approach is.

To recap: the most important part to me is prioritizing Pure (& versatile) functions.

Of course virtually no app is 100% pure functions, at some point you need to persist data or get data over a network; the key is when that happens it's tightly scoped w/ Single Purpose in mind. Ultimately any transformations, validation, or security checks should be alongside your side-effect functions - helps you avoid tightly coupled logic.