planetscale/database-js

Add CJS distribution to allow both NextJS and Node usage

Izhaki opened this issue · 5 comments

Izhaki commented

This package is esm-only at the time of writing. This is a good call and a growing trend.

However, NextJS does not currently fully support Node’s explicit file extension pattern for esm.

This means that it is impossible to:

  • Use this library with NextJS
  • AND write tests that use the library running in Node (such as E2E/Integration tests that require DB mutations).

Further, despite esm seen as the de-facto better future, its support is still limited, with many issues still to iron out.

A CJS distribution along an ESM one, would ensure that developers do not have to waste time on esm friction (I did, and it took 10 hours).

The diagram below depicts the issue.

nextjs-esm-only

We primarily use this driver in the Next.js app that serves https://app.planetscale.com. Can you provide more detail on what code isn't working?

Izhaki commented

We also use the driver so we can have edge API routes speaking to the DB as part of a NextJS app.

But production is only one side of the coin. On the other side are development and testing.

Example

Can you provide more detail on what code isn't working?

// app/pages/api/auth/login.js
import addUser from 'db/addUser';
// db/addUser
import { client } from './client';
// db/client
import { Client } from '@planetscale/database';

Now if we work it backwards:

  • @planetscale/database is esm only, so with Node you must configure the project package with "type": "module", or you'll get Error [ERR_REQUIRE_ESM]: Must use import to load ES Module.
  • But with "type": "module" the module specifier in db/addUser needs to change to ./client.js, which NextJS can't handle.

Possible Workaround

To be fair, If instead of:

import { Client } from '@planetscale/database';

One writes:

const { Client } = await import('@planetscale/database');

Then it should (haven't tried) build alright both with NextJS and on Node CJS. But one will have to commit the whole import chain (from the API route) to dynamic imports. Not classy, but at least when used on edge functions (but not in Node-runtime API routes) the teardown is immediate so no real penalty in doing this (I believe).

But that's a workaround alright...

Testing

We want coverage for that part of the system - the scripts that mutate the DB. Sure, much of it can be done in E2E tests, but some scenarios are rare, and impractical to reproduce via the UI (or API) - you need a non-production environment with state control.

Related, during development we want to see exactly what the driver returns, and somehow be able to simulate errors for error handling.

Further, for integration tests, we also want to control the DB state - nuking all tables (which is not a public API) and installing fixtures. We can use a CJS complaint library for that, like Prisma, but that's the RY in DRY - we already have most of the functionality written using the PScale driver.

There's also security tests like SQL injection - you REALLY want these going through the actual production code and not a look alike.

The takeaway is that you are likely to need Node to do some testing - no problem if you don't go through the same script as NextJS (because you can set it up to be purely ESM), but since NextJS does not support Node's ESM module specifiers (not without a really ugly hack), you can't go through the same scripts, which pretty much means you can test this part of the system.

Closing Thoughts

You can rightly argue that there's nothing wrong with this library - it works on Node, and it works on Edge Runtime.

But it locks Node usage to ESM, which doesn't marry with NextJS:

  • It will take NextJS some time to fully implement the fullySpecified config option (currently experimental).
  • It will take NextJS some time to take it out of experimental.
  • Even then using module specifiers throughout the app will probably be considered "adventurous".
  • It will take a few years (if at all, but I believe eventually it will happen), for the whole community to align around the ESM standard - specifically its requirement for valid URLs in the browser and explicit file extensions in Node.

Against all this stands adding CJS distribution to the library.

Izhaki commented

I've just tried the dynamic import option and it works alright (see code below).

I'm inclined to say that as I'm the only one who has experienced this issue, seems sensible to defer the addition of a CJS distribution for now - if others will have similar issues, we can reconsider.

I'm closing this issue and the linked PR. Owners are free to reopen at will.

export async function getExecute() {
  const { Client } = await import('@planetscale/database');

  const client = new Client({
    url: process.env['DATABASE_URL'],
  });

  const conn = client.connection();

  async function execute<T>(query: string, params?: object): Promise<T[]> {
    const result = await conn.execute(query, params);
    return result.rows as T[];
  }

  return execute;
}
Izhaki commented

Didn't work after all. Ended up using https://www.npmjs.com/package/fix-esm

Didn't work for me either, but fix-esm did