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 - Version 2 #1566

Closed
wants to merge 2 commits into from

Conversation

klodoma
Copy link
Contributor

@klodoma klodoma commented May 1, 2024

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

Discussion here: #1482

Changes:

  • by default look for backstop.js or backstop.json
  • adjusted the init function to handle properly the initialization
    • also added the option to init with --js flag
  • updated readme

Comment on lines -117 to +119
Example: Create a backstop.config.js
Example: Create a `backstop.config.js`

Choose a reason for hiding this comment

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

I might be wrong, but I don't think this PR would automatically find a backstop.config.js so this example should be changed to

Example: Create a `backstop.js`

Copy link
Owner

Choose a reason for hiding this comment

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

This is correct -- but I don't think this is necessary.

Copy link
Owner

@garris garris left a comment

Choose a reason for hiding this comment

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

I have no problem removing the file extension so that advanced users can easily interchange file types implicitly. This is how require() was intended to work.

The intention of init is to...

  1. make it easy for novice users to get started with Backstop JS
  2. enable basic automated sanity testing

I am not inclined to add another file format generation to init -- I don't think this moves the objectives forward. If anything -- awareness of js config option to me is more of a problem -- as measured by users' questions in this area. Perhaps emphasising this in the docs or in example code would more directly address the issue?

@klodoma
Copy link
Contributor Author

klodoma commented May 2, 2024

I am not inclined to add another file format generation to init -- I don't think this moves the objectives forward.

@garris

Imo I think this project should switch from .json to .js config file. We'll keep the "backward" compatibility through the require feature.

So, my proposal would be:

  • we keep the config as it is, this will work for both .json and .js
  • init will generate a .js file by default - so almost going back to the current code
  • we update the documentation so that .js is by default used
  • old projects that use the .json format will still work.
  • everybody's happy and this would be a "evolution"

@solomonhawk
Copy link

solomonhawk commented May 2, 2024

Any consideration for using https://github.com/cosmiconfig/cosmiconfig? It might be nice to offload the work to a separate well-tested, fully-featured library and allow backdrop to give end-users the flexibility that they might expect coming from other ecosystem packages that use cosmiconfig.

@maxfenton
Copy link

@garris @klodoma Just wondering if any version of this solution can be merged.

@garris
Copy link
Owner

garris commented May 21, 2024

@maxfenton I would be willing to merge a PR for just this change. It opens up possibilities with the lightest touch.

image

@klodoma
Copy link
Contributor Author

klodoma commented May 22, 2024

@garris
That would be the second PR.

I didn't hat time anymore I'll have a second look and let you know.

@garris
Copy link
Owner

garris commented May 23, 2024

I just made the change. Hope this is helpful. Will go out in next release. 944884b

@klodoma klodoma closed this May 23, 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.

None yet

4 participants