Skip to content

feat: add the .ageneratorrc file for setting the generator Config. #1485

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.

Sign up for GitHub

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 6 commits into
base: master
Choose a base branch
from
Open

feat: add the .ageneratorrc file for setting the generator Config. #1485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
607973f
Added the first version of the .ageneratorrc
ItshMoh Apr 9, 2025
6c9a2c1
moved to fs.access
ItshMoh Apr 9, 2025
37eeb86
changed the comments and error statements
ItshMoh Apr 9, 2025
a9ac501
changes the error string to match the test error string
ItshMoh Apr 9, 2025
9dde88d
Merge branch 'master' into alter-config
ItshMoh Apr 9, 2025
e13b9b1
Merge branch 'master' into alter-config
asyncapi-bot Apr 9, 2025
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 apps/generator/lib/filtersRegistry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ module.exports.registerFilters = async (nunjucks, templateConfig, templateDir, f
await registerLocalFilters(nunjucks, templateDir, filtersDir);
registerConfigFilters(nunjucks, templateDir, templateConfig);

// Register Nunjucks filters from the 'nunjucks-filters' module without needing to list them explicitly in package.json
// Register Nunjucks filters from the 'nunjucks-filters' module without needing to list them in package.json or .ageneratorrc file.
addFilters(nunjucks, nunjucksFilters);
};

Expand Down
32 changes: 23 additions & 9 deletions apps/generator/lib/generator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ class Generator {
enumerable: true,
get() {
if (!self.templateConfig.parameters?.[key]) {
throw new Error(`Template parameter "${key}" has not been defined in the package.json file under generator property. Please make sure it's listed there before you use it in your template.`);
throw new Error(`Template parameter "${key}" has not been defined in the .ageneratorrc file. Please make sure it's listed there before you use it in your template.`);
}
return templateParams[key];
}
Expand Down Expand Up @@ -968,16 +968,30 @@ class Generator {
*/
async loadTemplateConfig() {
try {
const configPath = path.resolve(this.templateDir, CONFIG_FILENAME);
if (!fs.existsSync(configPath)) {
this.templateConfig = {};
return;
// First, check for .ageneratorrc file
const rcConfigPath = path.resolve(this.templateDir, '.ageneratorrc');

try {
await fs.promises.access(rcConfigPath);
const yaml = await readFile(rcConfigPath, { encoding: 'utf8' });
const yamlConfig = require('js-yaml').load(yaml);
this.templateConfig = yamlConfig || {};
} catch (accessError) {
// File doesn't exist, fallback to package.json
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, it would be better to explicitly check for the presence of the file and doing conditional code, instead of relying on throwing errors and catching them.

Reasons:
1- You have enclosed multiple operations in the try block, not only the access function. If one of the other operations produce an error such as readFile (unrelated to file not existing) or the require().load(), it could be wrongly interpreted as file not existing. This is also for the second try/catch below.

2- There are multiple nested try/catch, and I believe this reduces readability and makes it harder to maintain. It would be much easier if it was a linear flow with conditionals instead.

Proposal: replace .access with a method that checks for the presence of file and returns a boolean.

Copy link
Contributor

@nightknighto nightknighto Apr 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I looked up the methods and it seems that the promisified exist method is deprecated.
I guess if we don't want to use existsSync, then this is a good option: https://stackoverflow.com/a/65446343/16761632

Copy link
Contributor

@nightknighto nightknighto Apr 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

On another note, while looking through the docs, I found out an explicit note to not use .access to check if the file exists before reading, instead reading directly and handling the error if it doesn't exist: https://nodejs.org/api/fs.html#fsaccesspath-mode-callback

const configPath = path.resolve(this.templateDir, CONFIG_FILENAME);

try {
await fs.promises.access(configPath);
const json = await readFile(configPath, { encoding: 'utf8' });
const generatorProp = JSON.parse(json).generator;
this.templateConfig = generatorProp || {};
} catch (packageAccessError) {
// package.json doesn't exist or can't be accessed
this.templateConfig = {};
}
}

const json = await readFile(configPath, { encoding: 'utf8' });
const generatorProp = JSON.parse(json).generator;
this.templateConfig = generatorProp || {};
} catch (e) {
console.error('Error loading template config:', e);
this.templateConfig = {};
}
await this.loadDefaultValues();
Expand Down
2 changes: 1 addition & 1 deletion apps/generator/test/generator.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe('Generator', () => {
expect(gen.forceWrite).toStrictEqual(true);
expect(gen.install).toStrictEqual(true);
expect(gen.compile).toStrictEqual(false);
expect(() => gen.templateParams.test).toThrow('Template parameter "test" has not been defined in the package.json file under generator property. Please make sure it\'s listed there before you use it in your template.');
expect(() => gen.templateParams.test).toThrow('Template parameter "test" has not been defined in the .ageneratorrc file. Please make sure it\'s listed there before you use it in your template.');

// Mock params on templateConfig so it doesn't fail.
gen.templateConfig.parameters = { test: {} };
Expand Down
Loading