I'm trying to write a module for "chainable" Express.js validation:

const validatePost = (req, res, next) => {
  validator.validate(req.body)
    .expect('name.first')
      .present('The parameter is required')
      .string('The parameter must be a string')
    .go(next);
};

router.post('/', validatePost, (req, res, next) => {
  return res.send('Validated!');
});

The code of validator.validate (simplified for brevity):

const validate = (data) => {

  let validation;

  const expect = (key) => {
    validation.key = key;

    // Here I get the actual value, but for testing purposes of .present() and
    // .string() chainable methods it returns a random value from a string,
    // not string and an undefined
    validation.value = [ 'foo', 123, void 0 ][Math.floor(Math.random() * 3)];
    return validation;
  };

  const present = (message) => {
    if (typeof validation.value === 'undefined') {
      validation.valid = false;
      validation.errors.push({ key: validation.key, message: message });
    }
    return validation;
  };

  const string = (message) => {
    if (typeof validation.value !== 'string') {
      validation.valid = false;
      validation.errors.push({ key: validation.key, message: message });
    }
    return validation;
  };

  const go = (next) => {
    if (!validation.valid) {
      let error = new Error('Validation error');
      error.name = 'ValidationError';
      error.errors = validation.errors;

      // I even wrap async callbacks in process.nextTick()
      process.nextTick(() => next(error));
    }
    process.nextTick(next);
  };

  validation = {
    valid: true,
    data: data,
    errors: [],
    expect: expect,
    present: present,
    string: string,
    go: go
  };

  return validation;

};

The code works fine for short chains, returning a proper error object. However if I chain a lot of methods, say:

const validatePost = (req, res, next) => {
  validator.validate(req.body)
    .expect('name.first')
      .present('The parameter is required')
      .string('The parameter must be a string')
    .expect('name.first') // Same for testing
      .present('The parameter is required')
      .string('The parameter must be a string')
    // [...] 2000 times
    .go(next);
};

Node.js throws RangeError: Maximum call stack size exceeded. Note that I wrapped my async callback .go(next) in a process.nextTick().

I'm trying to write a module for "chainable" Express.js validation:

const validatePost = (req, res, next) => {
  validator.validate(req.body)
    .expect('name.first')
      .present('The parameter is required')
      .string('The parameter must be a string')
    .go(next);
};

router.post('/', validatePost, (req, res, next) => {
  return res.send('Validated!');
});

The code of validator.validate (simplified for brevity):

const validate = (data) => {

  let validation;

  const expect = (key) => {
    validation.key = key;

    // Here I get the actual value, but for testing purposes of .present() and
    // .string() chainable methods it returns a random value from a string,
    // not string and an undefined
    validation.value = [ 'foo', 123, void 0 ][Math.floor(Math.random() * 3)];
    return validation;
  };

  const present = (message) => {
    if (typeof validation.value === 'undefined') {
      validation.valid = false;
      validation.errors.push({ key: validation.key, message: message });
    }
    return validation;
  };

  const string = (message) => {
    if (typeof validation.value !== 'string') {
      validation.valid = false;
      validation.errors.push({ key: validation.key, message: message });
    }
    return validation;
  };

  const go = (next) => {
    if (!validation.valid) {
      let error = new Error('Validation error');
      error.name = 'ValidationError';
      error.errors = validation.errors;

      // I even wrap async callbacks in process.nextTick()
      process.nextTick(() => next(error));
    }
    process.nextTick(next);
  };

  validation = {
    valid: true,
    data: data,
    errors: [],
    expect: expect,
    present: present,
    string: string,
    go: go
  };

  return validation;

};

The code works fine for short chains, returning a proper error object. However if I chain a lot of methods, say:

const validatePost = (req, res, next) => {
  validator.validate(req.body)
    .expect('name.first')
      .present('The parameter is required')
      .string('The parameter must be a string')
    .expect('name.first') // Same for testing
      .present('The parameter is required')
      .string('The parameter must be a string')
    // [...] 2000 times
    .go(next);
};

Node.js throws RangeError: Maximum call stack size exceeded. Note that I wrapped my async callback .go(next) in a process.nextTick().

• javascript
• node.js
• validation
• ecmascript-6

• Coincidentally I've been studying all night about various continuation passing styles and using trampolines in languages (like JavaScript) that don't support TCO. I was probably reading about process.nextTick the same time you posted this question when I learned that it's used to effectively create a while loop using the event queue ^_^ – Mulan Commented Dec 3, 2016 at 9:46

2 Answers 2

I didn't have a lot of time to look at this, but I did notice a pretty big problem. You have a single-branch if statement that leads to next being called twice when !validator.valid is true. In general, single-branch if statements are a code smell.

This might not be the reason you're experiencing a stack overflow, but it's a likely culprit.

(Code changes appear in bold)

const go = (next) => {
  if (!validation.valid) {
    let error = new Error('Validation error');
    error.name = 'ValidationError';
    error.errors = validation.errors;
    process.nextTick(() => next(error));
  }
  else {
    process.nextTick(next);
  }
};

Some people use return to cheat with if too. This also works, but it sucks

const go = (next) => {
  if (!validation.valid) {
    let error = new Error('Validation error');
    error.name = 'ValidationError';
    error.errors = validation.errors;
    process.nextTick(() => next(error));
    return; // so that the next line doesn't get called too
  }
  process.nextTick(next);
};

I think the entire go function is expressed better like this ...

const go = (next) => {
  // `!` is hard to reason about
  // place the easiest-to-understand, most-likely-to-happen case first
  if (validation.valid) {
    process.nextTick(next)
  }
  // very clear if/else branching
  // there are two possible outes and one block of code for each
  else {
    let error = new Error('Validation error');
    error.name = 'ValidationError';
    error.errors = validation.errors;
    // no need to create a closure here
    process.nextTick(() => next(error));
    process.nextTick(next, error);
  }
};

Other remarks

You have other single-branch if statements in your code too

const present = (message) => {
  if (typeof validation.value === 'undefined') {
    // this branch only performs mutations and doesn't return anything
    validation.valid = false;
    validation.errors.push({ key: validation.key, message: message });
  }
  // there is no `else` branch ...

  return validation;
};

This one is less offensive, but I still think it's harder to reason about once you gain an appreciation for if statements that always have an else. Consider the ternary operator (?:) that forces both branches. Also consider languages like Scheme where a True and False branch are always required when using if.

Here's how I'd write your present function

const present = (message) => {
  if (validation.value === undefined) {
    // True branch returns
    return Object.assign(validation, {
      valid: false,
      errors: [...validation.errors, { key: validation.key, message }]
    })
  }
  else {
    // False branch returns
    return validation
  }
};

It's an opinionated remark, but I think it's one worth considering. When you have to return to this code and read it later, you'll thank me. Of course once your code is in this format, you can sugar the hell out of it to remove a lot of syntactic boilerplate

const present = message =>
  validation.value === undefined
    ? Object.assign(validation, {
        valid: false,
        errors: [...validation.errors, { key: validation.key, message }]
      })
    : validation

Advantages

• Implicit return effectively forces you to use a single expression in your function – this means you cannot (easily) over-plicate your functions
• Ternary expression is an expression, not a statement – if does not have a return value so use of ternary works well with the implicit return
• Ternary expression limits you to one expression per branch – again, forces you to keep your code simple
• Ternary expression forces you to use both true and false branches so that you always handle both outes of the predicate

And yes, there's nothing stopping you from using () to bine multiple expressions into one, but the point isn't to reduce every function to a single expression – it's more of an ideal and nice to use when it works out. If at any time you feel readability was affected, you can resort to if (...) { return ... } else { return ... } for a familiar and friendly syntax/style.

Method Chaining Overflow

From your full code paste

validate({ name: { last: 'foo' }})
    // Duplicate this line ~2000 times for error
    .expect('name.first').present().string()
    .go(console.log);

You simply cannot chain that many methods in a single expression.

In an isolated test, we show that this has nothing to do with recursion or process.nextTick

class X {
  foo () {
    return this
  }
}

let x = new X()

x.foo().foo().foo().foo().foo().foo().foo().foo().foo().foo()
.foo().foo().foo().foo().foo().foo().foo().foo().foo().foo()
.foo().foo().foo().foo().foo().foo().foo().foo().foo().foo()
...
.foo().foo().foo().foo().foo().foo().foo().foo().foo().foo()

// RangeError: Maximum call stack size exceeded

Using 64-bit Chrome on OSX, the method chaining limit is 6253 before a stack overflow happens. This likely varies per implementation.

Lateral thinking

A method-chaining DSL seems like a nice way to specify validation properties for your data. It's unlikely you'd need to chain more than a couple dozen lines in a given validation expression, so you shouldn't be too worried about the limit.

Aside from that, a pletely different solution might be altogether better. One example that immediately es to mind is JSON schema. Instead of writing validation with code, would write it declaratively with data.

Here's a quick JSON schema example

{
  "title": "Example Schema",
  "type": "object",
  "properties": {
    "firstName": {
      "type": "string"
    },
    "lastName": {
      "type": "string"
    },
    "age": {
      "description": "Age in years",
      "type": "integer",
      "minimum": 0
    }
  },
  "required": ["firstName", "lastName"]
}

There's effectively no limit on how big your schema can be, so this should be suitable to solve your problem.

Other advantages

• Schema is portable so other areas of your app (eg testing) or other consumers of your data can use it
• Schema is JSON so it's a familiar format and users don't need to learn new syntax or API