fix(server): throw when encountering undefined positional arguments

[marshall007]

Fixes #951

Note: I had to manually run yarn add graphql postgraphile-core in order to get the tests running on my local machine. For whatever reason these dependencies were not installed with just yarn.

[benjie]

Hey @marshall007; you need to run prettier on src/postgraphile/postgraphile.ts; best way is yarn prettier:fix.

[benjie]

I'm going to classify this as a bugfix rather than a breaking change because it may help users find previously undetected errors in their code. Should they want to skip an arg they should instead pass null.

[benjie]

Notes to self:

• Check you can pass null for connection string and have it extracted from env
• Check that null for schema behaves reasonably
• Check that null for options behaves reasonably
• Check that CLI won't fail

[benjie]

Just handling edge cases now!

[benjie]

Suggested change

	incomingOptions = schemaOrOptions || {};
	if (typeof schemaOrOptions !== 'object') {
	throw new Error("Expected schema string/array or options object, instead received " + typeof schemaOrOptions);
	}
	incomingOptions = schemaOrOptions || {};

This'll handle things like:

postgraphile(pool, false) / postgraphile(pool, true) / postgraphile(pool, 0) / postgraphile(pool, true, {...}) / postgraphile(pool, 0, {...}) I think.

[benjie]

Maybe it'd be cleaner to restructure the code using arguments.length directly:

function postgraphile(a, b, c) {
  let pool = a;
  let schema;
  let options;
  if (arguments.length >= 1 && typeof a === 'undefined') {
    throw new Error("Pool argument passed to postgraphile was undefined; to use the defaults please pass `null` (or omit all arguments).");
  }
  if (arguments.length === 3) {
    if (!b) {
      throw new Error("Options specified, but schema omitted, pass your schema name or 'public'");
    }
    if (!c) {
      throw new Error("Third argument to postgraphile was falsy - please either omit it or pass an options object");
    }
    [schema, options] = [b, c];
  } else if (arguments.length === 2) {
    if (!b) {
      throw new Error("Second argument to postgraphile was falsy");
    }
    if (typeof b === 'string' || Array.isArray(b)) {
      schema = b
    } else if (typeof b === 'object') {
      options = b;
    } else {
      throw new Error("Invalid options passed to PostGraphile");
    }
  }
  // ...
}

I think I'll remove this flexibility in v5 and just go with 3 standard arguments like in the docs.

[marshall007]

@benjie I think I may have come up with a simpler stop-gap solution, but let me know if that doesn't work for you. As for breaking changes in v5, I'd be in favor of simplifying further to two arguments:

function postgraphile(poolConfig?: Pool | PoolConfig, options?: PostGraphileOptions) {
  // ...
}

... with options.schema as a nested property instead.

[benjie]

That makes a lot of sense 👍 for schema as an option.

[benjie]

Lovely 👍