Remove default config file extension · garris/BackstopJS@944884b

dgrebb · 2024-09-01T02:40:30Z

For 944884b (this conversation #1566) I believe this needs to be changed as well:

--- a/core/util/makeConfig.js
+++ b/core/util/makeConfig.js
@@ -26,7 +26,7 @@ function loadProjectConfig (command, options, config) {
       config.backstopConfigFileName = path.join(config.projectPath, customConfigPath);
     }
   } else {
-    config.backstopConfigFileName = path.join(config.projectPath, 'backstop.json');
+    config.backstopConfigFileName = path.join(config.projectPath, 'backstop');
   }

dgrebb · 2024-09-01T04:20:33Z

I think this may cause issues in the future. commonjs can indeed find the difference between json and js requires, however, it will get confused by files that lack extensions.

Integration tests are failing due because of this inability to differentiate (without extension):

❯ npm run integration-test

> backstopjs@6.3.23 integration-test
> rm -rf integrationTestDir && mkdir integrationTestDir && cd integrationTestDir && node ../cli/index.js init && node ../cli/index.js reference && node ../cli/index.js test && node -e "require('../')('test')" && npm --prefix ../ run -s success-message || npm --prefix ../ run -s fail-message

BackstopJS v6.3.23
COMMAND | Executing core for "init"
   init | Copying '/BackstopJS/capture/engine_scripts' to '/BackstopJS/integrationTestDir/backstop_data/engine_scripts'
   init | Configuration file written at /BackstopJS/integrationTestDir/backstop'
COMMAND | Command "init" successfully executed in [0.004s]
BackstopJS v6.3.23
Loading config:  /BackstopJS/integrationTestDir/backstop

/BackstopJS/integrationTestDir/backstop:2
  "id": "backstop_default",
      ^

SyntaxError: Unexpected token ':'
    at wrapSafe (node:internal/modules/cjs/loader:1469:18)
    at Module._compile (node:internal/modules/cjs/loader:1491:20)
    at Module._extensions..js (node:internal/modules/cjs/loader:1691:10)
    at Module.load (node:internal/modules/cjs/loader:1317:32)
    at Module._load (node:internal/modules/cjs/loader:1127:12)
    at TracingChannel.traceSync (node:diagnostics_channel:315:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:217:24)
    at Module.require (node:internal/modules/cjs/loader:1339:12)
    at require (node:internal/modules/helpers:126:16)
    at loadProjectConfig (/BackstopJS/core/util/makeConfig.js:46:20)

dgrebb · 2024-09-01T09:08:00Z

@garris FWIW... when I saw the integration-test error, I did start to unravel how it could be addressed. But we would maybe want to release-candidate and RFC something like this.

ChatGPT says:

To handle a configuration file that could be either a JavaScript file or a JSON file without an extension, you can adjust your code to try loading it as a JavaScript file first, and if that fails, attempt to load it as JSON. Here’s how you can modify the loadProjectConfig function:

const path = require('path');
const extendConfig = require('./extendConfig');
const fs = require('fs');

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

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

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

  let customConfigPath = options && (options.backstopConfigFilePath || options.configPath);
  if (options && typeof options.config === 'string' && !customConfigPath) {
    customConfigPath = options.config;
  }

  if (customConfigPath) {
    if (path.isAbsolute(customConfigPath)) {
      config.backstopConfigFileName = customConfigPath;
    } else {
      config.backstopConfigFileName = path.join(config.projectPath, customConfigPath);
    }
  } else {
    config.backstopConfigFileName = path.join(config.projectPath, 'backstop');
  }

  let userConfig = {};
  const CMD_REQUIRES_CONFIG = !NON_CONFIG_COMMANDS.includes(command);
  if (CMD_REQUIRES_CONFIG) {
    // This flow is confusing -- is checking for !config.backstopConfigFileName more reliable?
    if (options && typeof options.config === 'object' && options.config.scenarios) {
      console.log('Object-literal config detected.');
      if (options.config.debug) {
        console.log(JSON.stringify(options.config, null, 2));
      }
      userConfig = options.config;
    } else if (config.backstopConfigFileName) {
      try {
        // Try loading as a JavaScript module
        delete require.cache[require.resolve(config.backstopConfigFileName)];
        console.log('Loading JS config: ', config.backstopConfigFileName, '\n');
        userConfig = require(config.backstopConfigFileName);
      } catch (err) {
        // If that fails, try loading as JSON
        console.log('Failed to load as JS, attempting as JSON.');
        const configContent = fs.readFileSync(config.backstopConfigFileName, 'utf8');
        userConfig = JSON.parse(configContent);
      }
    }
  }

  return userConfig;
}

function makeConfig(command, options) {
  const config = {};

  config.args = options || {};

  config.backstop = path.join(__dirname, '../..');
  config.projectPath = projectPath(config);
  config.perf = {};

  const userConfig = Object.assign({}, loadProjectConfig(command, options, config));

  return extendConfig(config, userConfig);
}

module.exports = makeConfig;

Explanation:

Try Loading as JavaScript: The code first attempts to load the configuration as a JavaScript module using require. If this works, it proceeds as expected.
Fallback to JSON: If loading as JavaScript fails (likely due to a syntax error when it tries to parse JSON as JS), the code then attempts to read the file as a plain text file and parse it as JSON.
File System Check: The fs.readFileSync method is used to read the file contents, and JSON.parse is used to parse it if it’s JSON.

This adjustment ensures that your code can handle configuration files without an extension, regardless of whether they are JavaScript or JSON.

garris · 2024-09-05T18:20:57Z

@dgrebb Thank you for looking into this.

Just to be clear -- WRT config.backstopConfigFileName = path.join(config.projectPath, 'backstop')

The intention is to allow a user to supply a backstop.json or backstop.js config file (with suffix included). The intention is not to allow a user to supply an un-suffix'd backstop config file.

I believe per this https://nodejs.org/api/modules.html#file-modules -- require looks for .js and then .json file extensions when an extension is not supplied to the require() method.

I think this may cause issues in the future. commonjs can indeed find the difference between json and js requires, however, it will get confused by files that lack extensions.

dgrebb · 2024-09-06T04:10:26Z

Where it gets tricky is initializing a new project, which will read from the default config value:

File: cli/index.js
15:       config: 'backstop'

and go on to makeConfig with that value:

File: core/util/makeConfig.js
53: function makeConfig (command, options) {
54:   const config = {};
55: 
56:   config.args = options || {};

and go on to generate an extension-less backstop configuration file...

promises.push(fs.copy(config.captureConfigFileNameDefault, config.backstopConfigFileName).then...

The good news is the integration test caught this :) Should we get those integration tests running on push to any branch 😉 ?

garris · 2024-09-07T23:07:25Z

Ok -- wow. great catch!

I just reverted the change. I see that the currently running tests don't validate the init flow. See, this is why they should revoke my git license. 🤦

Sorry, Where are those integration tests you're talking about? I know we looked at them before.

-Original file line number
+Diff line change
@@ Expand Up / @@ -12,7 +12,7 @@ function main () { @@
         boolean: ['h', 'help', 'v', 'version', 'i', 'docker'],
         string: ['config'],
         default: {
-          config: 'backstop.json'
+          config: 'backstop'
         }
       });
@@ Expand Down @@

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Commit

There are no files selected for viewing

6 comments on commit `944884b`

dgrebb commented on `944884b` Sep 1, 2024

dgrebb commented on `944884b` Sep 1, 2024 •

edited

Loading

dgrebb commented on `944884b` Sep 1, 2024 •

edited

Loading

garris commented on `944884b` Sep 5, 2024

dgrebb commented on `944884b` Sep 6, 2024

garris commented on `944884b` Sep 7, 2024

Commit

There are no files selected for viewing

6 comments on commit 944884b

dgrebb commented on 944884b Sep 1, 2024

Choose a reason for hiding this comment

dgrebb commented on 944884b Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

dgrebb commented on 944884b Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

Explanation:

garris commented on 944884b Sep 5, 2024

Choose a reason for hiding this comment

dgrebb commented on 944884b Sep 6, 2024

Choose a reason for hiding this comment

garris commented on 944884b Sep 7, 2024

Choose a reason for hiding this comment

6 comments on commit `944884b`

dgrebb commented on `944884b` Sep 1, 2024

dgrebb commented on `944884b` Sep 1, 2024 •

edited

Loading

dgrebb commented on `944884b` Sep 1, 2024 •

edited

Loading

garris commented on `944884b` Sep 5, 2024

dgrebb commented on `944884b` Sep 6, 2024

garris commented on `944884b` Sep 7, 2024