feat: Add TQL package (published as @vercel/tql)

[tcc-sejohnson]

See details in the package README and in #494.

POST-MERGE:

• Replace @sejohnson/tql with @vercel/tql in feat: Better templating for @vercel/postgres #494
• Declare a Changesets link for @vercel/tql => @vercel/postgres after (or when) feat: Better templating for @vercel/postgres #494 merges
• Write a Tinybird adapter?

[changeset-bot]

🦋 Changeset detected

Latest commit: 5d4a302

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@vercel/tql	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
vercel-storage-next-integration-test-suite	✅ Ready (Inspect)	Visit Preview	Nov 30, 2023 11:13pm

[adriancooney]

What is happening here? Are we expecting these constructors to throw errors and fail the tests?

[tcc-sejohnson]

These are typing tests -- they purely make sure that the types for this error constructor are working correctly. This is how tests are generally constructed when you want to only test typings -- it's how they're set up in DefinitelyTyped, for example -- you do something that should cause a type error, then assert that it is causing a type error.

[adriancooney]

I'm not a big fan of committing these to code unless absolutely necessary but I am acutely aware of how annoying this typing can be. An alternative that simplifies the typing but makes things slightly more verbose could be to create error class for each error type. For example:

export class TqlError extends Error {}
export class UntemplatedSqlCallTqlError extends TqlError {}
export class DialectMethodNotImplementedTqlError extends TqlError {
  constructor(method: string) {
    super(`The dialect you are using does not implement this method: ${method}`);
  } 
}

It also allows for super simple error discrimination for users:

import { DialiectMethodNotImplementedTqlError } from "@vercel/tql";

try {
  // ..
} catch(err) {
  if (err instanceof DialectMethodNotImplementedTqlError) {
    // .. do work
  }

  throw err;
}

[tcc-sejohnson]

I actually think the ergonomics of subclasses are significantly worse for the user in most cases, though I'm willing to be swayed. The main reason I've gone this route is because it makes this possible:

catch (e) {
  // as soon as you type the opening quote, TypeScript gives you
  // every possible error from this library, requiring no documentation
  if (e instanceof TqlError && e.code === 'untemplated_sql_call')
}

catch (e) {
  if (e instanceof TqlError) {
    switch (e.type) {
      case 'untemplated_sql_call':
        break;
      default:
        console.log('Impossible if exhaustive');
    }
  }
}

as opposed to:

catch (e) {
  if (e instanceof SomeErrorFromThisLibrary) {
    // I have to both know that this error type exists and import it,
    // for every kind of error I'd like to handle
    // which makes documentation hard and discoverability even harder
  }
}

catch (e) {
  // oops, there's actually no way to switch over error types at all, 
  //I'd have to import every class I want to check for and if/elseif them. 
  //There's no way to make sure I'm handling every error type, 
  // and TypeScript gives me no help in making sure things are exhaustive.
}

I'm not particularly worried about this comment, though I'll look into whether there's a way to fix it -- I just couldn't find one in the limited time I had!

[adriancooney]

Ah we can still do best of both worlds! The super-class TqlError constructor can require a code parameter which the subclass DialectMethodNotImplementedTqlError can provide.

class TqlError extends Error {
	code: string;

	constructor(code: string, message: string) {
		super(message);
		this.code = code;
 	}
}

class DialectMethodNotImplementedTqlError extends TqlError {
  constructor() {
  	super("dialect_method_not_implemented", <message>)
  }
}

It's a matter of preference though I think. I generally prefer to discriminate errors using instanceof.

[tcc-sejohnson]

Right, but the problem here is that TypeScript is still dumb about them:

if (tqlError.code === '') // typescript has no idea what these can be

and additionally, even if I do know the code of an error:

if (tqlError.code === 'illegal_query_recursion') {
  // typescript has no way to connect this back to "oh, then the error is actually a `TqlIllegalRecursionError`
}

The authorship experience is also somewhat better when using a single error class, as typing:

throw new TqlError('

is all you need to do to see the entire list of errors it's possible to throw from the library, as opposed to having to type Error and hope VSCode's intellisense offers to import the right one 😆

[adriancooney]

I agree this rule is unnecessary but maybe for the sake of clean code we can be a little more verbose?

Suggested change

	// eslint-disable-next-line @typescript-eslint/unbound-method -- this rule is idiotic, this is a static method
	.map(PostgresDialect.escapeIdentifier)
	.map(column => PostgresDialect.escapeIdentifier(column))

(same with down below)

[tcc-sejohnson]

The point at which ESLint makes clean code less clean to avoid disabling the rule making the clean code error is the point at which ESLint has outlived its usefulness...

Fixed, begrudgingly!

[adriancooney]

Suggested change

	for (const param of vals.values) {
	const paramNumber = this.appendToParams(param);
	queryItems.push(`$${paramNumber}`);
	}
	this.appendToQuery(queryItems.join(', '));
	for (const param of vals.values) {
	this.parameter(param);
	this.appendToQuery(", ");
	}

Slight simplification. Feel free to ignore.

[tcc-sejohnson]

I'm not sure this would work -- wouldn't it cause there to be a trailing comma at the end of the list?

[adriancooney]

Can this be replaced with a call to this.list?

[tcc-sejohnson]

Hmm, you know, that might work in practice, but I think it might grow better if we leave it separate -- I'm not very confident the syntaxes are always the same -- and additionally, we'd have to turn the entry/column map into a TqlList before passing it to this.list, which probably would end up being more code anyway.

[adriancooney]

Are we wrapping this value in quotes in the query? If not, is there a reason we call this string? It feels like raw or something along those lines might be a more apt name.

[tcc-sejohnson]

It's called a TqlTemplateString (see the JSDoc on that class) -- though I wouldn't be against renaming it! TemplateStrings are only created by the develper-typed portions of query and fragment objects -- i.e. in:

const q = query`SELECT * FROM user WHERE user_id = ${1234} AND LOWER(user_name) LIKE 'jingleheimer%'`

You'd get this AST:

const ast = new TqlQuery([
  new TqlTemplateString('SELECT * FROM user WHERE user_id = '),
  new TqlParameter(1234),
  new TqlTemplateString(" AND LOWER(user_name) LIKE 'jingleheimer%'"),
])

Since we prevent query and fragment from being called in any way other than a template expression, we know these are always safe to append to the query directly, as they only could have been written by the developer. (The one exception is unsafe, which creates a TqlTemplateString directly, but... well, it's in the name. It's unsafe.)

[tcc-sejohnson]

Update: I realized there were a bunch of places where the slug for this method was just string (probably what you were pointing out), so I updated them all to be templateString, which, in addition to the JSDoc on that class, should make things clearer.

[adriancooney]

Should identifiers be plural here? I know the input can be a list but I think the output is singular. For example, "myschema"."mytable" is a singular identifier. Given that, is identifier (singular) more appropriate here?

[tcc-sejohnson]

I originally had both identifier (accepted a single value) and identifiers (accepts multiple values), but I combined them into one -- mirroring the VALUES clause in case, where even if you only have one value it's still VALUES. In this case, it can be called both ways:

identifiers(['hello', 'world', "table_a.column_name"]); // output when compiled to Postgres: "hello", "world", "table_a"."column_name"
identifiers('hello'); // output when compiled to Postgres: "hello"

I dunno, it seems weird to have two exports just to have the ability to call it with a single value vs. an array, and I'm not sure if making the single export singular or plural is more weird. Thoughts?

[tcc-sejohnson]

I think I may be leaning toward identifier which accepts either a single value or array, though it's still kinda weird either way...

[adriancooney]

It's unfortunate that list and set in this context here are very different than your typical list/set and might confuse users (e.g. Array vs Set). Is it worth trying to separate the concepts of list/set and list/sql-update-set? Maybe list becomes join or array? (join is actually pretty common).

[tcc-sejohnson]

I actually hate this just as much as you do! I struggled with list a lot and landed on it because:

• It doesn't match the semantics of join (you can't specify a delimiter, and it's doing more than just joining, it's actually substituting stuff into your query for you)
• JavaScript doesn't have lists (Python does, but every other programming language I know of calls 'em arrays)
• array would be even more confusing (especially since many SQL dialects have some sort of ARRAY or JSON_ARRAY keyword as well)

set is, IMO, the most confusing, because... well, JavaScript has a Set already. Hmm.

Here's a different idea. The goal with the names of these was to stick as closely as possible to the names of the clauses they output into your SQL. Here's an example -- if I'm writing the following SQL:

INSERT INTO users ("name", "dob") VALUES ("vercelliott", "2023-01-01")

The way that "sounds" when I write it is "INSERT INTO users VALUES". And so, the TQL for it is:

query`INSERT INTO users ${values(user)}`;

It "sounds" the same when you read it.

Here's an idea that might kill two birds with one stone: I have this weird problem with the fact that, just by looking at it, you can't tell which functions of the exported API create queries and which ones go inside queries:

import {
  query,
  fragment,
  identifiers,
  list,
  values,
  set,
  unsafe, 
} from '@vercel/tql';

query and fragment are "special" in that they're tagged templates, not "normal" functions. All of the other functions are meant to be used "inside" these templates. What if we mirrored the usage of these clauses in SQL, where we typically capitalize key words?:

import {
  query,
  fragment,
  IDENTIFIERS,
  LIST,
  VALUES,
  SET,
  UNSAFE, 
} from '@vercel/tql';

They'd look more at home in the SQL, and you'd also know they're a bit "special" -- they're supposed to be used inside your template:

query`INSERT INTO users ${VALUES(user)}`;

It'd also kinda solve our issue above -- no one will confuse SET with Set, as things aren't typically capitalized in JavaScript -- but they are in SQL.

[tcc-sejohnson]

This works really well in the SET case:

UPDATE users SET name = $1 WHERE user_id = $2;

becomes:

query`UPDATE users ${SET(updatedUser)} WHERE user_id = ${1234}`;

which "reads" the same, meaning it'll be easy to remember / write.

[tcc-sejohnson]

Implemented this in #507 for an example.

[adriancooney]

Overall this is really great. Some small notes about the exported API and missing tests. Great work @tcc-sejohnson!