graphile / migrate

Opinionated SQL-powered productive roll-forward migration tool for PostgreSQL.
MIT License
751 stars 58 forks source link

Support .cjs config files #129

Closed guillaumervls closed 3 years ago

guillaumervls commented 3 years ago

Description

Hello,

This is a quick fix the use of JS config files in projects where "type": "module" is used in package.json (which will me more and more frequent now that Node 10 obsolete). Cheers! The right way might be to support it via import

Performance impact

None

Security impact

None

Checklist

(not sure if that's something worth mentioning in README):

benjie commented 3 years ago

Could you complete this with a .mjs option? await import(...)

guillaumervls commented 3 years ago

I've added it, but I don't know how to do the correct switch between require and import. For now I've put a test on the file extension, but in the case of .js the correct call depends on the type field in package.json (import for module and require otherwise).

benjie commented 3 years ago

in the case of .js the correct call depends on the type field in package.json (import for module and require otherwise).

We could scan for this I suppose. It feels a bit expensive though since we might have to walk up a directory tree looking for a package.json which might not exist (e.g. if you're using migrate standalone). I think just requiring .mjs for now is the least painful option.

guillaumervls commented 3 years ago

Yes. I actually thought there could have been another solution than scanning the file system, some kind of global variable that Node sets when it's in "module" mode, that I wasn't aware of.

benjie commented 3 years ago

Yes. I actually thought there could have been another solution than scanning the file system, some kind of global variable that Node sets when it's in "module" mode, that I wasn't aware of.

I tried looking for this, but I've not managed to find it either. I have found an interesting behaviour though - in a commonJS file if require.main is undefined then you were started in module mode. This doesn't necessarily mean that this is what package.json states though. To see this:

// foo.mjs
import * as assert from 'assert';

assert.ok(true);
console.log("OK");
await import("./common.cjs");
// foo.cjs
const assert = require('assert');

assert.ok(true);
console.log("OK");
require("./common.cjs");
// common.cjs
console.log("Hello from common");
console.log(require.main);

Now run node foo.mjs and node foo.cjs and spot the difference.

benjie commented 3 years ago

Wait... Are we overthinking it?

import() can be used from both CJS and MJS files. And it can process both CJS and MJS files. So if import is available and extension is .js we can just import it and Node'll figure it out?

(As you may guess, I'm not keen to merge this until I'm certain the direction we're taking is correct.)

guillaumervls commented 3 years ago

Hi @benjie,

Sorry for the (very) late reply. I think you're right, I dit it here https://github.com/graphile/migrate/pull/140 (I closed this PR by accident)

Cheers !