Skip to content

feat(config): allow external config files in js/ts format #68

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
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
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ jobs:
strategy:
fail-fast: false
matrix:
node: [14, 16]
node: [18, 20, 22]
os: [ubuntu-latest]

steps:
Expand Down
6 changes: 3 additions & 3 deletions cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const parseArgs = (cmd) => {
const directory = cmd.path || parent.path;
return {
...cmd,
configFile: cmd.configFile,
configFile: cmd.config,
environment: cmd.env || parent.env,
directory: directory ? path.resolve(directory) : undefined,
sourceEnvironmentId: cmd.sourceEnvironmentId || parent.sourceEnvironmentId,
Expand Down Expand Up @@ -107,7 +107,7 @@ program
.action(
actionRunner(async (cmd) => {
const config = await getConfig(parseArgs(cmd || {}));
const verified = await askMissing(config);
const verified = await askMissing(config, ['accessToken', 'spaceId', 'environmentId', 'directory']);
await fetchMigration({ ...verified, contentType: cmd.contentType });
})
);
Expand All @@ -124,7 +124,7 @@ program
.action(
actionRunner(async (cmd) => {
const config = await getConfig(parseArgs(cmd || {}));
const verified = await askMissing(config);
const verified = await askMissing(config, ['accessToken', 'spaceId', 'environmentId', 'directory']);
await createMigration(verified);
})
);
Expand Down
57 changes: 53 additions & 4 deletions lib/config.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const path = require('path');
const inquirer = require('inquirer');
const { createJiti } = require('jiti');
const mergeOptions = require('merge-options').bind({ ignoreUndefined: true });
const { cosmiconfig } = require('cosmiconfig');

Expand All @@ -11,6 +12,8 @@ const STORAGE_CONTENT = 'content';
const STATE_SUCCESS = 'success';
const STATE_FAILURE = 'failure';

const jiti = createJiti(__filename);

/**
* Get configuration
* @param {Object} args
Expand All @@ -26,7 +29,7 @@ const getConfig = async (args) => {
const environmentOptions = {
spaceId: process.env.CONTENTFUL_SPACE_ID,
environmentId: process.env.CONTENTFUL_ENVIRONMENT_ID,
accessToken: process.env.CONTENTFUL_MANAGEMENT_TOKEN,
managementToken: process.env.CONTENTFUL_MANAGEMENT_TOKEN,
host: process.env.CONTENTFUL_HOST,
};

Expand All @@ -40,7 +43,7 @@ const getConfig = async (args) => {
const { managementToken, activeSpaceId, activeEnvironmentId, host } = config || {};
contentfulCliOptions = {
spaceId: activeSpaceId,
accessToken: managementToken,
managementToken: managementToken,
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably also should be documented, that we now expect managementToken as the name (but still support accessToken?)

host,
};
}
Expand All @@ -52,7 +55,7 @@ const getConfig = async (args) => {
try {
// get configuration from migrations rc file
const explorer = cosmiconfig('migrations');
const explorerResult = args.config ? await explorer.load(args.config) : await explorer.search();
const explorerResult = await explorer.search();
if (explorerResult !== null) {
const { config, filepath } = explorerResult || {};

Expand All @@ -69,7 +72,53 @@ const getConfig = async (args) => {
console.log('Error:', error.message);
}

return mergeOptions(defaultOptions, contentfulCliOptions, environmentOptions, configFileOptions, args || {});
let userConfigFileOptions = {};
if (args.config) {
try {
let config = {};
let filepath = '';
if (args.config.endsWith('.js') || args.config.endsWith('.ts')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When we now support ts files, that must be documented in the readme

config = await jiti.import(path.resolve(args.config));
filepath = jiti.esmResolve(path.resolve(args.config));
} else {
const explorer = cosmiconfig('migrations');
const explorerResult = await explorer.load(args.config);
config = explorerResult.config;
filepath = explorerResult.filepath;
}
if (config) {
userConfigFileOptions = {
directory: path.resolve(path.dirname(filepath || ''), args.directory || 'migrations'),
...(config || {}),
};

if (userConfigFileOptions.directory && !path.isAbsolute(userConfigFileOptions.directory)) {
userConfigFileOptions.directory = path.resolve(path.dirname(filepath || ''), userConfigFileOptions.directory);
}
Comment on lines +90 to +97
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole getConfig function got quite long and is hard to digest.
It seems this block is the same logic as above where configFileOptions is created. I think it could help to extract this code into a little helper function.


if (configFileOptions.directory) {
delete userConfigFileOptions.directory;
}
Comment on lines +99 to +101
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear why the config directory takes precedence over the user config directory. Would be nice to have a comment that explains that.

}
} catch (error) {
console.log('Error:', error.message);
}
}

const result = mergeOptions(
defaultOptions,
contentfulCliOptions,
environmentOptions,
configFileOptions,
userConfigFileOptions,
args || {}
);

if (result.managementToken) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be done to help with compatibility.
But could it not be that some user config set the accessToken?
So probably better to only overwrite it if it is not set?

result.accessToken = result.managementToken;
}

return result;
};

const getPromts = (data) => {
Expand Down
15 changes: 15 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
"author": "Ben Zörb <benjamin.zoerb@jvm.de>",
"license": "MIT",
"engines": {
"node": ">=14"
"node": ">=18"
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to increment the version with all those changes.
Is it done later when the whole thing is released?
This is a breaking change, is it not?
So we would require a new mayor version, right?

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, would be minor but as we increased the required node version this needs to be major. Version will be bumped in the release process

},
"keywords": [
"contentful",
Expand All @@ -51,6 +51,7 @@
"fs-extra": "^10.1.0",
"globby": "^12.0.2",
"inquirer": "^8.2.0",
"jiti": "^2.4.2",
"markdown-table": "^3.0.4",
"merge-options": "^3.0.4",
"mustache": "^4.2.0",
Expand Down