Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add support for ESM config files #987

Merged
merged 4 commits into from
Jan 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion .babelrc
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,17 @@
],
"ignore": [
"src/assets"
]
],
"overrides": [{
"test": "./src/helpers/import-helper.js",
"presets": [
["@babel/env", {
"targets": {
"node": "10.0"
},
"shippedProposals": true,
"exclude": ["proposal-dynamic-import"],
}]
],
}]
}
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ pids

# Editor files
.vscode
.idea

# Directory for instrumented libs generated by jscoverage/JSCover
lib-cov
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"pg": "latest",
"pg-hstore": "latest",
"prettier": "^2.4.1",
"semver": "^7.3.5",
"sequelize": "^6.9.0",
"sqlite3": "latest",
"through2": "^4.0.2"
Expand Down
60 changes: 28 additions & 32 deletions src/helpers/config-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,45 +5,41 @@ import _ from 'lodash';
import { promisify } from 'util';
import helpers from './index';
import getYArgs from '../core/yargs';
import importHelper from './import-helper';

const args = getYArgs().argv;

const api = {
config: undefined,
rawConfig: undefined,
error: undefined,
init() {
return Promise.resolve()
.then(() => {
let config;

if (args.url) {
config = api.parseDbUrl(args.url);
} else {
try {
config = require(api.getConfigFile());
} catch (e) {
api.error = e;
}
}
return config;
})
.then((config) => {
if (typeof config === 'object' || config === undefined) {
return config;
} else if (config.length === 1) {
return promisify(config)();
} else {
return config();
}
})
.then((config) => {
api.rawConfig = config;
})
.then(() => {
// Always return the full config api
return api;
});
async init() {
let config;

try {
if (args.url) {
config = api.parseDbUrl(args.url);
} else {
const module = await importHelper.importModule(api.getConfigFile());
config = await module.default;
}
} catch (e) {
api.error = e;
}

if (typeof config === 'function') {
// accepts callback parameter
if (config.length === 1) {
config = await promisify(config)();
} else {
// returns a promise.
config = await config();
}
}

api.rawConfig = config;

return api;
},
getConfigFile() {
if (args.config) {
Expand Down
1 change: 1 addition & 0 deletions src/helpers/dummy-file.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
// this file is imported by import-helper to detect whether dynamic imports are supported.
35 changes: 35 additions & 0 deletions src/helpers/import-helper.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
async function supportsDynamicImport() {
try {
// imports are cached.
// no need to worry about perf here.
// Don't remove .js: extension must be included for ESM imports!
await import('./dummy-file.js');
return true;
} catch (e) {
return false;
}
}

/**
* Imports a JSON, CommonJS or ESM module
* based on feature detection.
*
* @param modulePath path to the module to import
* @returns {Promise<unknown>} the imported module.
*/
async function importModule(modulePath) {
// JSON modules are still behind a flag. Fallback to require for now.
// https://nodejs.org/api/esm.html#json-modules
if (!modulePath.endsWith('.json') && (await supportsDynamicImport())) {
return import(modulePath);
}

// mimics what `import()` would return for
// cjs modules.
return { default: require(modulePath) };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes that the module only has a default export, and is not compatible with named exports.

For the purposes of the config that's fine, because the config is exported on the default export when is ESM compatible, but this is not true for all cases.

Which makes me feel like it would be better to invoke supportsDynamicImport in config-helper.js and either use the import helper, or straight up require.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand what you mean.

If the user is using named exports then the platform is compatible with ESM and import() will be used, which supports named exports.

If they're using CJS, there are no named-exports.

Native import() of a CJS file already returns { default: module.exports }.

Copy link
Contributor

@corevo corevo Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand what you mean, I think I didn't fully understand what you were saying in #982

But basically you wanted to write a node 10 compatible importModule function, while I was under the impression that you were going to drop support for node 10.

EDIT: by you, I mean the sequelize team

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll indeed drop Node 10 for v7, but I don't think we want to wait with fixing this until the core sequelize package is ready for that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, makes sense, I think we can resolve this then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, so that's an approval from you? Then I think we can merge and set up a new release

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes exactly :) The point was to replicate import() using require, the code that uses import-helper is then responsible for properly using the module

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point was to replicate import() using require

For sequelize v7, will this solution be dropped and using the native import be supported instead? Along with .sequelizerc no longer having module.exports ? Should I open an issue for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the next major will drop support for node 10 so import() will always be used.

Supporting ESM in .sequelizerc could already be done in this version though. It should be a small change too.
Simply need to make this line use the new import helper

? JSON.parse(JSON.stringify(require(rcFileResolved)))
+ support searching for .sequelizerc + .sequelizerc.json + .sequelizerc.cjs + .sequelizerc.mjs + .sequelizerc.js.

(feel free to open an issue)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've openeded #989 , which is only for v7.
I think current version is fine as is, would prefer focusing on v7 since sequelize v7 is almost ready.

}

module.exports = {
supportsDynamicImport,
importModule,
};
69 changes: 67 additions & 2 deletions test/db/migrate.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const expect = require('expect.js');
const Support = require(__dirname + '/../support');
const helpers = require(__dirname + '/../support/helpers');
const gulp = require('gulp');
const semver = require('semver');
const _ = require('lodash');

[
Expand All @@ -25,7 +26,7 @@ const _ = require('lodash');
let configPath = 'config/';
let migrationFile = options.migrationFile || 'createPerson';
const config = _.assign({}, helpers.getTestConfig(), options.config);
let configContent = JSON.stringify(config);
let configContent = JSON.stringify(config, null, 2);

if (!migrationFile.match(/\.(cjs|ts)$/)) {
migrationFile = migrationFile + '.js';
Expand Down Expand Up @@ -70,7 +71,11 @@ const _ = require('lodash');
it('creates a SequelizeMeta table', function (done) {
const self = this;

prepare(() => {
prepare((e) => {
if (e) {
return done(e);
}

helpers.readTables(self.sequelize, (tables) => {
expect(tables).to.have.length(2);
expect(tables).to.contain('SequelizeMeta');
Expand Down Expand Up @@ -293,6 +298,58 @@ describe(Support.getTestDialectTeaser('db:migrate'), () => {
});
});

describeOnlyForESM(Support.getTestDialectTeaser('db:migrate'), () => {
describe('with config.mjs', () => {
const prepare = function (callback) {
const config = helpers.getTestConfig();
const configContent = 'export default ' + JSON.stringify(config);
let result = '';

return gulp
.src(Support.resolveSupportPath('tmp'))
.pipe(helpers.clearDirectory())
.pipe(helpers.runCli('init'))
.pipe(helpers.removeFile('config/config.json'))
.pipe(helpers.copyMigration('createPerson.js'))
.pipe(helpers.overwriteFile(configContent, 'config/config.mjs'))
.pipe(helpers.runCli('db:migrate --config config/config.mjs'))
.on('error', (e) => {
callback(e);
})
.on('data', (data) => {
result += data.toString();
})
.on('end', () => {
callback(null, result);
});
};

it('creates a SequelizeMeta table', function (done) {
prepare((e) => {
if (e) {
return done(e);
}

helpers.readTables(this.sequelize, (tables) => {
expect(tables).to.have.length(2);
expect(tables).to.contain('SequelizeMeta');
done();
});
});
});

it('creates the respective table', function (done) {
prepare(() => {
helpers.readTables(this.sequelize, (tables) => {
expect(tables).to.have.length(2);
expect(tables).to.contain('Person');
done();
});
});
});
});
});

describe(Support.getTestDialectTeaser('db:migrate'), () => {
describe('with config.json and url option', () => {
const prepare = function (callback) {
Expand Down Expand Up @@ -525,3 +582,11 @@ describe(Support.getTestDialectTeaser('db:migrate'), () => {
});
});
});

function describeOnlyForESM(title, fn) {
if (semver.satisfies(process.version, '^12.20.0 || ^14.13.1 || >=16.0.0')) {
describe(title, fn);
} else {
describe.skip(title, fn);
}
}