refactor(config): make logger required on ValidatedConfig (#3457)

this commit adds the `Logger` instance to `ValidatedConfig`, requiring
the latter to hold an instance of the former.

the `validateConfig` function has been altered to accept an instance of
`LoadConfigInit`, the minimal configuration object that is used to
bootstrap a stencil task. this allows a central place for an unvalidated
configuration to be validated. Tather than semi-validating a config and
mutating it after the fact as a part of the load process (which is what
was previously done), all required fields are set in one place*

the `setupTestingConfig` function signature was altered to accept a
`ValidatedConfig` rather than a `Config`, as it's only caller has access
to a `ValidatedConfig`. this allows us to simplify some of the logic for
generating a validated config for testing purposes.

\* for cases where the config does not pass through `validateConfig`,
such as `runTask`, we will attempt to pull a `Logger` off the
unvalidated config, or create one if one does not exist

this commit introduces a mock creation function, `mockLoadConfigInit` to
init the configuration file that used to bootstrap a task. this mock
creator is used throughout the test suite, in order to provide a
`LoadConfigInit` entity for `validateConfig`

this commit does surface the face that `validateConfig` is used
throughout the testing suite, and that the suite(s) should be refactored
to use more specific functions under test, rather than calling the
larger `validateConfig` function. this is being considered
'out-of-scope' & saved for a future day

this commit removes a flaky test that was discovered in refactoring the
existing test suites. a top level configuration entity was not being
properly reset between test runs, causing the test to fail.

looking at the test, it no longer tests a valid path in the codebase -
in running coverage before and after this change (with jest's cache
disabled), we see no decrease in code coverage

this commit removes `Logger` instances from telemetry-related calls. as
a part of the larger effort to make `Logger` always available on the
`ValidatedConfig` entity, we will always be able to use the instance
that is stored on the configuration, allowing us to decrease the arity
of the fn signature



* review(ap): remove unneeded logger value

* review(ap): inline config var

src/cli/run.ts

@@ -15,7 +15,7 @@ import { taskServe } from './task-serve';
 import { taskTest } from './task-test';
 import { taskTelemetry } from './task-telemetry';
 import { telemetryAction } from './telemetry/telemetry';
-import { ValidatedConfig } from '../declarations';
+import { createLogger } from '../compiler/sys/logger/console-logger';
 
 export const run = async (init: d.CliInitOptions) => {
   const { args, logger, sys } = init;
@@ -72,7 +72,7 @@ export const run = async (init: d.CliInitOptions) => {
     loadedCompilerLog(sys, logger, flags, coreCompiler);
 
     if (task === 'info') {
-      await telemetryAction(sys, { flags: { task: 'info' }, outputTargets: [] }, logger, coreCompiler, async () => {
+      await telemetryAction(sys, { flags: { task: 'info' }, logger }, coreCompiler, async () => {
         await taskInfo(coreCompiler, sys, logger);
       });
       return;
@@ -100,7 +100,7 @@ export const run = async (init: d.CliInitOptions) => {
 
     await sys.ensureResources({ rootDir: validated.config.rootDir, logger, dependencies: dependencies as any });
 
-    await telemetryAction(sys, validated.config, logger, coreCompiler, async () => {
+    await telemetryAction(sys, validated.config, coreCompiler, async () => {
       await runTask(coreCompiler, validated.config, task, sys);
     });
   } catch (e) {
@@ -118,7 +118,9 @@ export const runTask = async (
   task: d.TaskCommand,
   sys?: d.CompilerSystem
 ) => {
-  const strictConfig: ValidatedConfig = { ...config, flags: { ...config.flags } ?? { task } };
+  const logger = config.logger ?? createLogger();
+  const strictConfig: d.ValidatedConfig = { ...config, flags: { ...config.flags } ?? { task }, logger };
+
   strictConfig.outputTargets = strictConfig.outputTargets || [];
 
   switch (task) {
@@ -136,7 +138,7 @@ export const runTask = async (
       break;
 
     case 'help':
-      await taskHelp(strictConfig.flags, config.logger, sys);
+      await taskHelp(strictConfig.flags, strictConfig.logger, sys);
       break;
 
     case 'prerender':
@@ -150,7 +152,7 @@ export const runTask = async (
     case 'telemetry':
       // TODO(STENCIL-148) make this parameter no longer optional, remove the surrounding if statement
       if (sys) {
-        await taskTelemetry(strictConfig.flags, sys, config.logger);
+        await taskTelemetry(strictConfig.flags, sys, strictConfig.logger);
       }
       break;
 
@@ -163,8 +165,10 @@ export const runTask = async (
       break;
 
     default:
-      config.logger.error(`${config.logger.emoji('❌ ')}Invalid stencil command, please see the options below:`);
-      await taskHelp(strictConfig.flags, config.logger, sys);
+      strictConfig.logger.error(
+        `${strictConfig.logger.emoji('❌ ')}Invalid stencil command, please see the options below:`
+      );
+      await taskHelp(strictConfig.flags, strictConfig.logger, sys);
       return config.sys.exit(1);
   }
 };

src/cli/task-build.ts

@@ -26,7 +26,7 @@ export const taskBuild = async (coreCompiler: CoreCompiler, config: d.ValidatedC
 
     // TODO(STENCIL-148) make this parameter no longer optional, remove the surrounding if statement
     if (sys) {
-      await telemetryBuildFinishedAction(sys, config, config.logger, coreCompiler, results);
+      await telemetryBuildFinishedAction(sys, config, coreCompiler, results);
     }
 
     await compiler.destroy();

src/cli/telemetry/telemetry.ts

@@ -10,14 +10,12 @@ import { isOutputTargetHydrate, WWW } from '../../compiler/output-targets/output
  *
  * @param sys The system where the command is invoked
  * @param config The config passed into the Stencil command
- * @param logger The tool used to do logging
  * @param coreCompiler The compiler used to do builds
  * @param result The results of a compiler build.
  */
 export async function telemetryBuildFinishedAction(
   sys: d.CompilerSystem,
   config: d.ValidatedConfig,
-  logger: d.Logger,
   coreCompiler: CoreCompiler,
   result: d.CompilerBuildResults
 ) {
@@ -32,23 +30,22 @@ export async function telemetryBuildFinishedAction(
   const data = await prepareData(coreCompiler, config, sys, result.duration, component_count);
 
   await sendMetric(sys, config, 'stencil_cli_command', data);
-  logger.debug(`${logger.blue('Telemetry')}: ${logger.gray(JSON.stringify(data))}`);
+
+  config.logger.debug(`${config.logger.blue('Telemetry')}: ${config.logger.gray(JSON.stringify(data))}`);
 }
 
 /**
  * A function to wrap a compiler task function around. Will send telemetry if, and only if, the machine allows.
  *
  * @param sys The system where the command is invoked
  * @param config The config passed into the Stencil command
- * @param logger The tool used to do logging
  * @param coreCompiler The compiler used to do builds
  * @param action A Promise-based function to call in order to get the duration of any given command.
  * @returns void
  */
 export async function telemetryAction(
   sys: d.CompilerSystem,
   config: d.ValidatedConfig,
-  logger: d.Logger,
   coreCompiler: CoreCompiler,
   action?: d.TelemetryCallback
 ) {
@@ -78,7 +75,7 @@ export async function telemetryAction(
   const data = await prepareData(coreCompiler, config, sys, duration);
 
   await sendMetric(sys, config, 'stencil_cli_command', data);
-  logger.debug(`${logger.blue('Telemetry')}: ${logger.gray(JSON.stringify(data))}`);
+  config.logger.debug(`${config.logger.blue('Telemetry')}: ${config.logger.gray(JSON.stringify(data))}`);
 
   if (error) {
     throw error;

src/cli/telemetry/test/telemetry.spec.ts

@@ -2,7 +2,7 @@ import type * as d from '../../../declarations';
 import * as telemetry from '../telemetry';
 import * as shouldTrack from '../shouldTrack';
 import { createSystem } from '../../../compiler/sys/stencil-sys';
-import { mockValidatedConfig } from '@stencil/core/testing';
+import { mockLogger, mockValidatedConfig } from '@stencil/core/testing';
 import * as coreCompiler from '@stencil/core/compiler';
 import { anonymizeConfigForTelemetry } from '../telemetry';
 import { DIST, DIST_CUSTOM_ELEMENTS, DIST_HYDRATE_SCRIPT, WWW } from '../../../compiler/output-targets/output-utils';
@@ -31,7 +31,7 @@ describe('telemetryBuildFinishedAction', () => {
       duration: 100,
     } as d.CompilerBuildResults;
 
-    await telemetry.telemetryBuildFinishedAction(sys, config, config.logger, coreCompiler, results);
+    await telemetry.telemetryBuildFinishedAction(sys, config, coreCompiler, results);
     expect(spyShouldTrack).toHaveBeenCalled();
 
     spyShouldTrack.mockRestore();
@@ -57,7 +57,7 @@ describe('telemetryAction', () => {
       })
     );
 
-    await telemetry.telemetryAction(sys, config, config.logger, coreCompiler, () => {});
+    await telemetry.telemetryAction(sys, config, coreCompiler, () => {});
     expect(spyShouldTrack).toHaveBeenCalled();
 
     spyShouldTrack.mockRestore();
@@ -71,7 +71,7 @@ describe('telemetryAction', () => {
       })
     );
 
-    await telemetry.telemetryAction(sys, config, config.logger, coreCompiler, async () => {
+    await telemetry.telemetryAction(sys, config, coreCompiler, async () => {
       new Promise((resolve) => {
         setTimeout(() => {
           resolve(true);
@@ -142,6 +142,7 @@ describe('prepareData', () => {
       flags: {
         args: [],
       },
+      logger: mockLogger(),
     };
 
     sys = createSystem();
@@ -182,6 +183,7 @@ describe('prepareData', () => {
       flags: {
         args: [],
       },
+      logger: mockLogger(),
       outputTargets: [{ type: 'www', baseUrl: 'https://example.com', serviceWorker: { swDest: './tmp' } }],
     };
 
@@ -228,6 +230,7 @@ describe('prepareData', () => {
       flags: {
         args: [],
       },
+      logger: mockLogger(),
       outputTargets: [{ type: 'www', baseUrl: 'https://example.com', serviceWorker: { swDest: './tmp' } }],
     };
 

src/compiler/config/load-config.ts

@@ -6,7 +6,6 @@ import type {
   UnvalidatedConfig,
 } from '../../declarations';
 import { buildError, catchError, hasError, isString, normalizePath } from '@utils';
-import { createLogger } from '../sys/logger/console-logger';
 import { createSystem } from '../sys/stencil-sys';
 import { dirname } from 'path';
 import { IS_NODE_ENV } from '../sys/environment';
@@ -71,7 +70,7 @@ export const loadConfig = async (init: LoadConfigInit = {}): Promise<LoadConfigR
 
     unknownConfig.config.sys = sys;
 
-    const validated = validateConfig(unknownConfig.config);
+    const validated = validateConfig(unknownConfig.config, init);
     results.diagnostics.push(...validated.diagnostics);
     if (hasError(results.diagnostics)) {
       return results;
@@ -87,7 +86,6 @@ export const loadConfig = async (init: LoadConfigInit = {}): Promise<LoadConfigR
       results.config.logLevel = 'info';
     }
 
-    results.config.logger = init.logger || results.config.logger || createLogger();
     results.config.logger.setLevel(results.config.logLevel);
 
     if (!hasError(results.diagnostics)) {

src/compiler/config/test/validate-config-sourcemap.spec.ts

@@ -2,6 +2,7 @@ import type * as d from '../../../declarations';
 import { createSystem } from '../../../compiler/sys/stencil-sys';
 import { loadConfig } from '../load-config';
 import path from 'path';
+import { mockLoadConfigInit } from '@stencil/core/testing';
 
 describe('stencil config - sourceMap option', () => {
   const configPath = require.resolve('./fixtures/stencil.config.ts');
@@ -11,7 +12,7 @@ describe('stencil config - sourceMap option', () => {
   /**
    * Test helper for generating default `d.LoadConfigInit` objects.
    *
-   * This function assumes the fields in the enclosing scope have ben initialized.
+   * This function assumes the fields in the enclosing scope have been initialized.
    * @param overrides the properties on the default `d.LoadConfigInit` entity to manually override
    * @returns the default configuration initialization object, with any overrides applied
    */
@@ -22,7 +23,8 @@ describe('stencil config - sourceMap option', () => {
       config: {},
       initTsConfig: true,
     };
-    return { ...defaults, ...overrides };
+
+    return mockLoadConfigInit({ ...defaults, ...overrides });
   };
 
   beforeEach(() => {