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.

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

Use any config files: backstop.json, backstop.js #1565

Closed
wants to merge 3 commits into from
Closed

Use any config files: backstop.json, backstop.js #1565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7be08b6
Use any config file: backstop.json, backstop.js
klodoma Apr 30, 2024
d0a1472
Use any config file: backstop.json, backstop.js
klodoma Apr 30, 2024
17a8045
Use any config file: backstop.json, backstop.js
klodoma Apr 30, 2024
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
4 changes: 1 addition & 3 deletions cli/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,7 @@ function main () {
const argsOptions = parseArgs(process.argv.slice(2), {
boolean: ['h', 'help', 'v', 'version', 'i', 'docker'],
string: ['config'],
default: {
config: 'backstop.json'
Copy link
Owner

@garris garris May 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@klodoma @maxfenton @dgrebb

FWIW: if we change line 15 to config: 'backstop' then require() will resolve this as a .js then a json file on its own. This would be the absolute simplest change. I don't think any other changes would be needed.

What do you guys think? Would this solve the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh, I didn't know that :) good point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@garris

Please have a look at this PR. I'll add more comments in the PR.
#1566

So probably we'll abandon this current PR.

Copy link

Choose a reason for hiding this comment

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

I will test both today, but I think the docs on the new PR still suggest a backstop.config.js filename and I'm not clear if resolve will find a file with .config in it.

I do like that this PR makes it clear what filenames are allowed and what order of priority they would take.

Copy link

Choose a reason for hiding this comment

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

I find this PR to be more elegant than v2 which adds a useJs flag that seems extraneous. I think the logic in this PR handles the case where someone has added both a backstop.js and backstop.json in the directory (the json is used because of array priority) and this PR handles more filenames and shows the order of them more explicitly.

That's just my opinion, but I'd vote for this PR over the other one, even though resolve can spot .json and .js

Copy link

Choose a reason for hiding this comment

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

(And I really appreciate the work in both PR's @klodoma!)

}
default: {}
});

// Catch errors from failing promises
Expand Down
40 changes: 26 additions & 14 deletions core/util/makeConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,32 +1,44 @@
const path = require('path');
const fs = require('fs');
const extendConfig = require('./extendConfig');
const logger = require('../util/logger')('init');

const NON_CONFIG_COMMANDS = ['init', 'version', 'stop'];

function projectPath (config) {
function projectPath () {
return process.cwd();
}

function loadProjectConfig (command, options, config) {
options = options || {}; // make sure options is an object

// TEST REPORT FILE NAME
const customTestReportFileName = options && (options.testReportFileName || null);
if (customTestReportFileName) {
config.testReportFileName = options.testReportFileName || null;
if (options.testReportFileName) {
config.testReportFileName = options.testReportFileName;
}

let customConfigPath = options && (options.backstopConfigFilePath || options.configPath);
if (options && typeof options.config === 'string' && !customConfigPath) {
customConfigPath = options.config;
// list of "config" files to search for
const configFiles = ['backstop.json', 'backstop.js'];
Copy link

@maxfenton maxfenton Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think all four of these are valid options:

Suggested change
const configFiles = ['backstop.json', 'backstop.js'];
const configFiles = ['backstop.json', 'backstop.config.json', 'backstop.js', 'backstop.config.js'];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxfenton Do you REALLY need the support backstop.config.*?

I personally don't mind if we add them, still I don't see it necessary.

@garris ? What do you say?

Copy link

Choose a reason for hiding this comment

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

@klodoma I have a coworker who — today — said he prefers it on his projects because it’s more clear when looking at the root of a project what's a config file. I'd feel bad if I didn't at least make this suggestion for his sake and since it’s probably the default some other users have been using with the config flag.

Copy link

Choose a reason for hiding this comment

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

And it’s the default name in the documentation here: https://github.com/garris/BackstopJS?tab=readme-ov-file#working-with-your-config-file


// If a config file is specified in the options, add it to the list
if (options.config && typeof options.config === 'string') {
configFiles.unshift(options.config);
configFiles.unshift(path.join(config.projectPath, options.config));
}

if (customConfigPath) {
if (path.isAbsolute(customConfigPath)) {
config.backstopConfigFileName = customConfigPath;
} else {
config.backstopConfigFileName = path.join(config.projectPath, customConfigPath);
// Searching for the first existing config file
for (const configFile of configFiles) {
const configPath = path.join(config.projectPath, configFile);
if (fs.existsSync(configPath)) {
config.backstopConfigFileName = configPath;
break; // Stop searching once a valid config file is found
}
} else {
config.backstopConfigFileName = path.join(config.projectPath, 'backstop.json');
}

if (!config.backstopConfigFileName) {
logger.error('No config file found in project path: ' + config.projectPath);
logger.error('Looked for: ' + configFiles.join(', '));
process.exit(1);
}

let userConfig = {};
Expand Down