feat: one config aggregator (#274)

* feat: use the same configAggregator everywhere

* chore: linter and current standards

* fix: support setting json = true via env

* feat: support json env in any case

* test: ut for jsonEnabled

* test: non-json cases

* chore: bump core

* chore: update jsdoc rules

* Revert "chore: update jsdoc rules"

This reverts commit 619b74a.

* style: indent on doc, turn off deprecated rule

* style: exclude example from jsdoc indent rule

* style: get through windows-only linter issue

.eslintrc.js

@@ -9,16 +9,10 @@
 // See more at https://github.com/forcedotcom/sfdx-dev-packages/tree/master/packages/dev-scripts
 
 module.exports = {
-  extends: ['eslint-config-salesforce-typescript', 'eslint-config-salesforce-license'],
+  extends: ['eslint-config-salesforce-typescript', 'eslint-config-salesforce-license', 'plugin:sf-plugin/library'],
+  root: true,
   rules: {
-    '@typescript-eslint/restrict-template-expressions': 'off',
-    '@typescript-eslint/explicit-function-return-type': 'off',
-    'jsdoc/check-indentation': 'off',
-    '@typescript-eslint/prefer-regexp-exec': 'off',
-    '@typescript-eslint/no-use-before-define': 'off',
-    '@typescript-eslint/camelcase': 'off',
-    'no-underscore-dangle': 'off',
-    '@typescript-eslint/require-await': 'off',
+    'jsdoc/newline-after-description': 'off',
+    'jsdoc/check-indentation': ['warn', { excludeTags: ['example'] }],
   },
-  root: true,
 };

messages/messages.md

@@ -93,7 +93,9 @@ This command is currently in beta. Any aspect of this command can change without
 
 # error.InvalidArgumentFormat
 
-Set varargs with this format: key=value or key="value with spaces".
+Error in the following argument
+%s
+Set varargs with this format: key=value or key="value with spaces"
 
 # error.DuplicateArgument
 

package.json

@@ -33,7 +33,7 @@
   ],
   "dependencies": {
     "@oclif/core": "^2.8.2",
-    "@salesforce/core": "^3.34.6",
+    "@salesforce/core": "^3.35.0",
     "@salesforce/kit": "^1.9.2",
     "@salesforce/ts-types": "^1.7.3",
     "chalk": "^4",
@@ -56,7 +56,8 @@
     "eslint-config-salesforce-typescript": "^1.1.1",
     "eslint-plugin-header": "^3.1.1",
     "eslint-plugin-import": "^2.27.5",
-    "eslint-plugin-jsdoc": "^40.3.0",
+    "eslint-plugin-jsdoc": "^43.1.1",
+    "eslint-plugin-sf-plugin": "^1.15.1",
     "husky": "^7.0.4",
     "mocha": "^9.1.3",
     "nyc": "^15.1.0",
@@ -144,4 +145,4 @@
       "output": []
     }
   }
-}
+}

src/flags/duration.ts

@@ -41,7 +41,9 @@ export type DurationFlagConfig = {
  * ```
  */
 export const durationFlag = Flags.custom<Duration, DurationFlagConfig>({
+  // eslint-disable-next-line @typescript-eslint/require-await
   parse: async (input, _, opts) => validate(input, opts),
+  // eslint-disable-next-line @typescript-eslint/require-await
   default: async (context) =>
     context.options.defaultValue ? toDuration(context.options.defaultValue, context.options.unit) : undefined,
 });

src/flags/salesforceId.ts

@@ -44,6 +44,7 @@ export type IdFlagConfig = {
  * ```
  */
 export const salesforceIdFlag = Flags.custom<string, IdFlagConfig>({
+  // eslint-disable-next-line @typescript-eslint/require-await
   parse: async (input, _ctx, opts) => validate(input, opts),
   char: 'i',
 });

src/sfCommand.ts

@@ -16,9 +16,7 @@ import {
   EnvironmentVariable,
   SfError,
   ConfigAggregator,
-  SfdxConfigAggregator,
 } from '@salesforce/core';
-import { env } from '@salesforce/kit';
 import { AnyJson } from '@salesforce/ts-types';
 import * as chalk from 'chalk';
 import { Progress, Prompter, Spinner, Ux } from './ux';
@@ -42,15 +40,15 @@ export const StandardColors = {
 /**
  * A base command that provided common functionality for all sf commands.
  * Functionality includes:
- *  - JSON support
- *  - progress bars
- *  - spinners
- *  - prompts
- *  - stylized output (JSON, url, objects, headers)
- *  - lifecycle events
- *  - configuration variables help section
- *  - environment variables help section
- *  - error codes help section
+ * - JSON support
+ * - progress bars
+ * - spinners
+ * - prompts
+ * - stylized output (JSON, url, objects, headers)
+ * - lifecycle events
+ * - configuration variables help section
+ * - environment variables help section
+ * - error codes help section
  *
  * All implementations of this class need to implement the run() method.
  *
@@ -163,29 +161,6 @@ export abstract class SfCommand<T> extends Command {
 
   /**
    * ConfigAggregator instance for accessing global and local configuration.
-   *
-   * NOTE: If the active executable is sfdx, this will be an instance of SfdxConfigAggregator, which supports
-   * the deprecated sfdx config vars like defaultusername, defaultdevhubusername, apiversion, etc. Otherwise,
-   * it will be an instance of ConfigAggregator will only supports the config vars introduce by @salesforce/core@v3.
-   *
-   * The executable is determined by `this.config.bin` which is supplied by the base oclif/core Command class. The value
-   * of `this.config.bin` will be the executable running (e.g. sfdx or sf) or, for local development (e.g. using bin/dev),
-   * it will be the value of oclif.bin in the plugin's package.json.
-   *
-   * If you need to write NUTS for a plugin that needs to work with both sets of config vars you can
-   * use set the `SF_USE_DEPRECATED_CONFIG_VARS` to `true` to force configAggregator to be an instance of SfdxConfigAggregator or
-   * `false` to force configAggregator to be an instance of ConfigAggregator.
-   *
-   * @example
-   * ```
-   * import { execCmd } from '@salesforce/cli-plugins-testkit';
-   * execCmd('config:set [email protected]', {
-   *   env: {
-   *     ...process.env,
-   *     SF_USE_DEPRECATED_CONFIG_VARS: true,
-   *   }
-   * })
-   * ```
    */
   public configAggregator!: ConfigAggregator;
 
@@ -209,6 +184,16 @@ export abstract class SfCommand<T> extends Command {
     return this.constructor as typeof SfCommand;
   }
 
+  public jsonEnabled(): boolean {
+    // https://developer.salesforce.com/docs/atlas.en-us.sfdx_setup.meta/sfdx_setup/sfdx_dev_cli_json_support.htm
+    // can come from either oclif's detection of the flag's presence and truthiness OR from the env
+    // unless it's been explicitly disabled on the command's statics
+    return (
+      super.jsonEnabled() ||
+      (this.statics.enableJsonFlag !== false &&
+        envVars.getString(EnvironmentVariable.SF_CONTENT_TYPE)?.toUpperCase() === 'JSON')
+    );
+  }
   /**
    * Log a success message that has the standard success message color applied.
    *
@@ -360,19 +345,15 @@ export abstract class SfCommand<T> extends Command {
   }
 
   public async _run<R>(): Promise<R> {
-    this.configAggregator =
-      this.config.bin === 'sfdx' ??
-      env.getBoolean('SF_USE_DEPRECATED_CONFIG_VARS') ??
-      env.getBoolean('SFDX_USE_DEPRECATED_CONFIG_VARS')
-        ? await SfdxConfigAggregator.create()
-        : await ConfigAggregator.create();
+    this.configAggregator = await ConfigAggregator.create();
 
     if (this.statics.requiresProject) {
       this.project = await this.assignProject();
     }
     if (this.statics.state === 'beta') {
       this.warn(messages.getMessage('warning.CommandInBeta'));
     }
+    // eslint-disable-next-line @typescript-eslint/require-await
     this.lifecycle.onWarning(async (warning: string) => {
       this.warn(warning);
     });
@@ -394,6 +375,7 @@ export abstract class SfCommand<T> extends Command {
         });
       });
 
+    // eslint-disable-next-line no-underscore-dangle
     return super._run<R>();
   }
 
@@ -430,6 +412,7 @@ export abstract class SfCommand<T> extends Command {
     }
   }
 
+  // eslint-disable-next-line @typescript-eslint/require-await
   protected async catch(error: Error | SfError | SfCommand.Error): Promise<SfCommand.Error> {
     // transform an unknown error into one that conforms to the interface
 
@@ -499,7 +482,7 @@ export abstract class SfCommand<T> extends Command {
    */
   protected formatError(error: SfCommand.Error): string {
     const colorizedArgs: string[] = [];
-    const errorCode = error.code ? ` (${error.code})` : '';
+    const errorCode = typeof error.code === 'string' || typeof error.code === 'number' ? ` (${error.code})` : '';
     const errorPrefix = `${StandardColors.error(messages.getMessage('error.prefix', [errorCode]))}`;
     colorizedArgs.push(`${errorPrefix} ${error.message}`);
     colorizedArgs.push(...this.formatActions(error.actions ?? []));

src/stubUx.ts

@@ -4,6 +4,7 @@
  * Licensed under the BSD 3-Clause license.
  * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
  */
+/* eslint-disable @typescript-eslint/explicit-function-return-type */
 
 import { SinonSandbox } from 'sinon';
 import { SfCommand } from './sfCommand';

test/unit/sfCommand.test.ts

@@ -10,6 +10,7 @@ import { TestContext } from '@salesforce/core/lib/testSetup';
 import { stubMethod } from '@salesforce/ts-sinon';
 import { expect } from 'chai';
 import { SfError } from '@salesforce/core';
+import { Config } from '@oclif/core/lib/interfaces';
 import { SfCommand } from '../../src/sfCommand';
 
 class TestCommand extends SfCommand<void> {
@@ -42,6 +43,68 @@ class TestCommand extends SfCommand<void> {
   }
 }
 
+class NonJsonCommand extends SfCommand<void> {
+  public static enableJsonFlag = false;
+  public async run(): Promise<void> {
+    await this.parse(TestCommand);
+  }
+}
+describe('jsonEnabled', () => {
+  beforeEach(() => {
+    delete process.env.SF_CONTENT_TYPE;
+  });
+
+  const oclifConfig = {} as unknown as Config;
+  it('not json', () => {
+    // @ts-expect-error not really an oclif config
+    const cmd = new TestCommand([], oclifConfig);
+    expect(cmd.jsonEnabled()).to.be.false;
+  });
+  it('json via flag but not env', () => {
+    // @ts-expect-error not really an oclif config
+    const cmd = new TestCommand(['--json'], oclifConfig);
+    expect(cmd.jsonEnabled()).to.be.true;
+  });
+  it('json via env but not flag', () => {
+    process.env.SF_CONTENT_TYPE = 'JSON';
+    // @ts-expect-error not really an oclif config
+    const cmd = new TestCommand([], oclifConfig);
+    expect(cmd.jsonEnabled()).to.be.true;
+  });
+  it('json via env lowercase', () => {
+    process.env.SF_CONTENT_TYPE = 'json';
+    // @ts-expect-error not really an oclif config
+    const cmd = new TestCommand([], oclifConfig);
+    expect(cmd.jsonEnabled()).to.be.true;
+  });
+  it('not json via env that is not json', () => {
+    process.env.SF_CONTENT_TYPE = 'foo';
+    // @ts-expect-error not really an oclif config
+    const cmd = new TestCommand([], oclifConfig);
+    expect(cmd.jsonEnabled()).to.be.false;
+  });
+  it('json via both flag and env', () => {
+    process.env.SF_CONTENT_TYPE = 'JSON';
+    // @ts-expect-error not really an oclif config
+    const cmd = new TestCommand(['--json'], oclifConfig);
+    expect(cmd.jsonEnabled()).to.be.true;
+  });
+
+  describe('non json command', () => {
+    it('non-json command base case', () => {
+      // @ts-expect-error not really an oclif config
+      const cmd = new NonJsonCommand([], oclifConfig);
+      expect(cmd.jsonEnabled()).to.be.false;
+    });
+    it('non-json command is not affected by env', () => {
+      process.env.SF_CONTENT_TYPE = 'JSON';
+      // @ts-expect-error not really an oclif config
+      const cmd = new NonJsonCommand([], oclifConfig);
+      expect(cmd.jsonEnabled()).to.be.false;
+    });
+  });
+});
+
 describe('info messages', () => {
   const $$ = new TestContext();
   beforeEach(() => {

test/unit/util.test.ts

@@ -13,9 +13,13 @@ import {
   OrgConfigProperties,
   SfdxPropertyKeys,
   SFDX_ALLOWED_PROPERTIES,
+  Messages,
 } from '@salesforce/core';
 import { parseVarArgs, toHelpSection } from '../../src/util';
 
+Messages.importMessagesDirectory(__dirname);
+const messages = Messages.loadMessages('@salesforce/sf-plugins-core', 'messages');
+
 describe('toHelpSection', () => {
   it('should produce help section for env vars', () => {
     const envVarSection = toHelpSection('ENV VAR SECTION', EnvironmentVariable.SFDX_ACCESS_TOKEN);
@@ -114,9 +118,9 @@ describe('parseVarArgs', () => {
   });
 
   it('should throw if invalid format', () => {
-    expect(() => parseVarArgs({ arg1: 'foobar' }, ['foobar', 'key1=value1', 'key2:value2'])).to.throw(
-      'Set varargs with this format: key=value or key="value with spaces". key2:value2'
-    );
+    const badArg = 'key2:value2';
+    const expectedError = messages.createError('error.InvalidArgumentFormat', [badArg]).message;
+    expect(() => parseVarArgs({ arg1: 'foobar' }, ['foobar', 'key1=value1', badArg])).to.throw(expectedError);
   });
 
   it('should throw if duplicates exist', () => {