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

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

Closed
wants to merge 3 commits into from

Conversation

klodoma
Copy link
Contributor

@klodoma klodoma commented Apr 30, 2024

Use any config files: backstop.json, backstop.js whichever is found first.

Discussion here: #1482

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

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?

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.

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

@maxfenton
Copy link

This code works for me locally and does exactly what I needed. Thank you.

@@ -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

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.

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.

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

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!)

@klodoma
Copy link
Contributor Author

klodoma commented May 2, 2024

I'll close this one. The V2 is the way to go.

@klodoma klodoma closed this May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants