Allow eval in watch mode

Author: Mrtenz

packages/snaps-cli/src/commands/watch/watch.test.ts

@@ -34,6 +34,8 @@ describe('watchHandler', () => {
     expect(listen).toHaveBeenCalledWith(config.server.port);
     expect(watch).toHaveBeenCalledWith(config, {
       spinner: expect.any(Object),
+      evaluate: true,
+      spinner: expect.any(Object),
     });
 
     expect(log).toHaveBeenCalledWith(

packages/snaps-cli/src/commands/watch/watch.ts

@@ -45,7 +45,7 @@ const steps: Steps<WatchContext> = [
   {
     name: 'Building the Snap bundle.',
     task: async ({ config, spinner }) => {
-      await watch(config, { spinner });
+      await watch(config, { spinner, evaluate: config.evaluate });
     },
   },
 ];

packages/snaps-cli/src/webpack/__snapshots__/config.test.ts.snap

@@ -2183,7 +2183,7 @@ exports[`getDefaultConfiguration returns the default Webpack configuration for t
   "plugins": [
     SnapsWebpackPlugin {
       "options": {
-        "eval": false,
+        "eval": true,
         "manifestPath": "/bar/snap.manifest.json",
         "writeManifest": true,
       },
@@ -2225,8 +2225,6 @@ exports[`getDefaultConfiguration returns the default Webpack configuration for t
     },
     SnapsWatchPlugin {
       "options": {
-        "bundle": "/bar/baz/bundle.js",
-        "evaluate": true,
         "files": [
           "/bar/snap.manifest.json",
         ],
@@ -2750,8 +2748,6 @@ exports[`getDefaultConfiguration returns the default Webpack configuration for t
     },
     SnapsWatchPlugin {
       "options": {
-        "bundle": "/bar/baz/bundle.js",
-        "evaluate": false,
         "files": [
           "/bar/snap.manifest.json",
         ],
@@ -2889,7 +2885,7 @@ exports[`getDefaultConfiguration returns the default Webpack configuration for t
   "plugins": [
     SnapsWebpackPlugin {
       "options": {
-        "eval": false,
+        "eval": true,
         "manifestPath": "/bar/snap.manifest.json",
         "writeManifest": true,
       },
@@ -2931,8 +2927,6 @@ exports[`getDefaultConfiguration returns the default Webpack configuration for t
     },
     SnapsWatchPlugin {
       "options": {
-        "bundle": "/bar/baz/bundle.js",
-        "evaluate": true,
         "files": [
           "/bar/snap.manifest.json",
         ],

packages/snaps-cli/src/webpack/config.ts

@@ -1,7 +1,6 @@
 import SnapsWebpackPlugin from '@metamask/snaps-webpack-plugin';
 import ForkTsCheckerWebpackPlugin from 'fork-ts-checker-webpack-plugin';
 import type { Ora } from 'ora';
-import { resolve } from 'path';
 import TerserPlugin from 'terser-webpack-plugin';
 import type { Configuration } from 'webpack';
 import { DefinePlugin, ProgressPlugin, ProvidePlugin } from 'webpack';
@@ -308,7 +307,7 @@ export async function getDefaultConfiguration(
         {
           manifestPath: config.manifest.path,
           writeManifest: config.manifest.update,
-          eval: !options.watch && options.evaluate,
+          eval: options.evaluate,
         },
         options.spinner,
       ),
@@ -355,8 +354,6 @@ export async function getDefaultConfiguration(
       options.watch &&
         new SnapsWatchPlugin(
           {
-            bundle: resolve(config.output.path, config.output.filename),
-            evaluate: options.evaluate,
             files: [config.manifest.path],
           },
           options.spinner,

packages/snaps-cli/src/webpack/plugins.test.ts

@@ -10,7 +10,6 @@ import {
   SnapsStatsPlugin,
   SnapsWatchPlugin,
 } from './plugins';
-import * as evalImplementation from '../commands/eval/implementation';
 import { compile, getCompiler } from '../test-utils';
 
 jest.dontMock('fs');
@@ -264,93 +263,6 @@ describe('SnapsWatchPlugin', () => {
     const close = promisify(watcher.close.bind(watcher));
     await close();
   });
-
-  it('evaluates the bundle if enabled', async () => {
-    const log = jest.spyOn(console, 'log').mockImplementation();
-    const evaluate = jest
-      .spyOn(evalImplementation, 'evaluate')
-      .mockImplementation();
-
-    const fileSystem = createFsFromVolume(new Volume());
-    const compiler = await getCompiler({
-      fileSystem,
-      config: {
-        plugins: [
-          new SnapsWatchPlugin({
-            bundle: '/output.js',
-            evaluate: true,
-            files: [],
-          }),
-        ],
-      },
-    });
-
-    // Wait for the initial compilation to complete.
-    const watcher = await new Promise<Watching>((resolve) => {
-      const innerWatcher = compiler.watch(
-        {
-          poll: 1,
-          ignored: ['/output.js'],
-        },
-        () => {
-          resolve(innerWatcher);
-        },
-      );
-    });
-
-    expect(log).toHaveBeenCalledTimes(1);
-    expect(log).toHaveBeenCalledWith(
-      expect.stringMatching(/Snap bundle evaluated successfully\./u),
-    );
-    expect(evaluate).toHaveBeenCalled();
-
-    const close = promisify(watcher.close.bind(watcher));
-    await close();
-  });
-
-  it('logs evaluation errors', async () => {
-    const log = jest.spyOn(console, 'log').mockImplementation();
-    const error = jest.spyOn(console, 'error').mockImplementation();
-    const evaluate = jest
-      .spyOn(evalImplementation, 'evaluate')
-      .mockRejectedValue(new Error('Evaluation error.'));
-
-    const fileSystem = createFsFromVolume(new Volume());
-    const compiler = await getCompiler({
-      fileSystem,
-      config: {
-        plugins: [
-          new SnapsWatchPlugin({
-            bundle: '/output.js',
-            evaluate: true,
-            files: [],
-          }),
-        ],
-      },
-    });
-
-    // Wait for the initial compilation to complete.
-    const watcher = await new Promise<Watching>((resolve) => {
-      const innerWatcher = compiler.watch(
-        {
-          poll: 1,
-          ignored: ['/output.js'],
-        },
-        () => {
-          resolve(innerWatcher);
-        },
-      );
-    });
-
-    expect(log).not.toHaveBeenCalled();
-    expect(evaluate).toHaveBeenCalled();
-    expect(error).toHaveBeenCalledWith(
-      expect.stringContaining('Evaluation error.'),
-    );
-
-    const close = promisify(watcher.close.bind(watcher));
-    await close();
-  });
 });
 
 describe('SnapsBuiltInResolver', () => {

packages/snaps-cli/src/webpack/plugins.ts

@@ -12,7 +12,6 @@ import type {
 import { WebpackError } from 'webpack';
 
 import { formatText, pluralize } from './utils';
-import { evaluate } from '../commands/eval';
 import { error, getErrorMessage, info, warn } from '../utils';
 
 export type SnapsStatsPluginOptions = {
@@ -151,18 +150,6 @@ export class SnapsStatsPlugin implements WebpackPluginInstance {
  * The options for the {@link SnapsWatchPlugin}.
  */
 export type SnapsWatchPluginOptions = {
-  /**
-   * The bundle path. This is the file that will be evaluated, if the `evaluate`
-   * option is set.
-   */
-  bundle?: string;
-
-  /**
-   * Whether to evaluate the bundle. This only applies if the `bundle` option is
-   * set.
-   */
-  evaluate?: boolean;
-
   /**
    * The extra files to watch.
    */
@@ -207,30 +194,9 @@ export class SnapsWatchPlugin implements WebpackPluginInstance {
         this.options.files?.forEach(
           fileDependencies.add.bind(fileDependencies),
         );
-
-        if (this.options.bundle && this.options.evaluate) {
-          await this.#safeEvaluate(this.options.bundle);
-        }
       },
     );
   }
-
-  /**
-   * Safely evaluate the bundle at the given path. If an error occurs, it will
-   * be logged to the console, rather than throwing an error.
-   *
-   * This function should never throw an error.
-   *
-   * @param bundlePath - The path to the bundle.
-   */
-  async #safeEvaluate(bundlePath: string) {
-    try {
-      await evaluate(bundlePath);
-      info(`Snap bundle evaluated successfully.`, this.#spinner);
-    } catch (evaluateError) {
-      error(evaluateError.message, this.#spinner);
-    }
-  }
 }
 
 /**

packages/snaps-utils/src/manifest/manifest.ts

@@ -6,7 +6,7 @@ import pathUtils from 'path';
 
 import type { SnapManifest } from './validation';
 import type { ValidatorResults } from './validator';
-import { hasFixes, runValidators } from './validator';
+import { isReportFixable, hasFixes, runValidators } from './validator';
 import type { ValidatorMeta, ValidatorReport } from './validator-types';
 import * as defaultValidators from './validators';
 import { deepClone } from '../deep-clone';
@@ -65,6 +65,12 @@ export type CheckManifestOptions = {
    * between `@metamask/snaps-utils` and `@metamask/snaps-rpc-methods`.
    */
   handlerEndowments?: Record<string, string | null>;
+
+  /**
+   * Whether the compiler is running in watch mode. This is used to determine
+   * whether to fix warnings or errors only.
+   */
+  watchMode?: boolean;
 };
 
 /**
@@ -99,6 +105,8 @@ export type WriteFileFunction = (path: string, data: string) => Promise<void>;
  * handlers and their respective permission name. This must be provided to avoid
  * circular dependencies between `@metamask/snaps-utils` and
  * `@metamask/snaps-rpc-methods`.
+ * @param options.watchMode - Whether the compiler is running in watch mode.
+ * This is used to determine whether to fix warnings or errors only.
  * @returns Whether the manifest was updated, and an array of warnings that
  * were encountered during processing of the manifest files.
  */
@@ -110,6 +118,7 @@ export async function checkManifest(
     writeFileFn = fs.writeFile,
     exports,
     handlerEndowments,
+    watchMode = false,
   }: CheckManifestOptions = {},
 ): Promise<CheckManifestResult> {
   const manifestPath = pathUtils.join(basePath, NpmSnapFileNames.Manifest);
@@ -169,8 +178,8 @@ export async function checkManifest(
     reports: validatorResults.reports,
   };
 
-  if (updateAndWriteManifest && hasFixes(manifestResults)) {
-    const fixedResults = await runFixes(validatorResults);
+  if (updateAndWriteManifest && hasFixes(manifestResults, watchMode)) {
+    const fixedResults = await runFixes(validatorResults, undefined, watchMode);
 
     if (fixedResults.updated) {
       manifestResults = fixedResults;
@@ -206,11 +215,13 @@ export async function checkManifest(
  *
  * @param results - Results of the initial run of validation.
  * @param rules - Optional list of rules to run the fixes with.
+ * @param errorsOnly - Whether to only run fixes for errors, not warnings.
  * @returns The updated manifest and whether it was updated.
  */
 export async function runFixes(
   results: ValidatorResults,
   rules?: ValidatorMeta[],
+  errorsOnly = false,
 ): Promise<CheckManifestResult> {
   let shouldRunFixes = true;
   const MAX_ATTEMPTS = 10;
@@ -230,7 +241,10 @@ export async function runFixes(
 
     let manifest = fixResults.files.manifest.result;
 
-    const fixable = fixResults.reports.filter((report) => report.fix);
+    const fixable = fixResults.reports.filter((report) =>
+      isReportFixable(report, errorsOnly),
+    );
+
     for (const report of fixable) {
       assert(report.fix);
       ({ manifest } = await report.fix({ manifest }));
@@ -244,7 +258,7 @@ export async function runFixes(
     fixResults.files.manifest.result = manifest;
 
     fixResults = await runValidators(fixResults.files, rules);
-    shouldRunFixes = hasFixes(fixResults);
+    shouldRunFixes = hasFixes(fixResults, errorsOnly);
   }
 
   const initialReports: (CheckManifestReport & ValidatorReport)[] = deepClone(