diff --git a/eslint.config.mjs b/eslint.config.mjs index 690b91d218..9589c3cefd 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -14,6 +14,7 @@ const config = createConfig([ '**/docs', '**/public', '.yarn', + 'packages/snaps-cli/src/commands/watch/__test__', '!packages/snaps-cli/src/commands/build', ], }, diff --git a/packages/snaps-cli/src/commands/watch/__test__/invalid/input.mjs b/packages/snaps-cli/src/commands/watch/__test__/invalid/input.mjs new file mode 100644 index 0000000000..fd7eba6e68 --- /dev/null +++ b/packages/snaps-cli/src/commands/watch/__test__/invalid/input.mjs @@ -0,0 +1,5 @@ +// This file is used for the E2E watch test. + +export const onRpcRequest = () => { + // no-op +} diff --git a/packages/snaps-cli/src/commands/watch/__test__/invalid/package.json b/packages/snaps-cli/src/commands/watch/__test__/invalid/package.json new file mode 100644 index 0000000000..4247538a43 --- /dev/null +++ b/packages/snaps-cli/src/commands/watch/__test__/invalid/package.json @@ -0,0 +1,4 @@ +{ + "name": "watch-e2e-test", + "version": "0.0.0" +} diff --git a/packages/snaps-cli/src/commands/watch/__test__/invalid/snap.config.ts b/packages/snaps-cli/src/commands/watch/__test__/invalid/snap.config.ts new file mode 100644 index 0000000000..ab5b08e0f9 --- /dev/null +++ b/packages/snaps-cli/src/commands/watch/__test__/invalid/snap.config.ts @@ -0,0 +1,11 @@ +import type { SnapConfig } from '@metamask/snaps-cli'; + +const config: SnapConfig = { + input: 'input.mjs', + output: { + path: './dist', + filename: 'bundle.js', + }, +}; + +export default config; diff --git a/packages/snaps-cli/src/commands/watch/__test__/invalid/snap.manifest.json b/packages/snaps-cli/src/commands/watch/__test__/invalid/snap.manifest.json new file mode 100644 index 0000000000..c4c8d53996 --- /dev/null +++ b/packages/snaps-cli/src/commands/watch/__test__/invalid/snap.manifest.json @@ -0,0 +1,18 @@ +{ + "version": "0.0.0", + "description": "E2E test for Snap `watch` command with invalid manifest", + "proposedName": "E2E Test", + "source": { + "shasum": "9r/u7rEURvdwOi8MZc9nccNVNdIFgcMXjQCGIn9Mx/s=", + "location": { + "npm": { + "filePath": "bundle.js", + "packageName": "watch-e2e-test", + "registry": "https://registry.npmjs.org/" + } + } + }, + "initialPermissions": {}, + "platformVersion": "9.3.0", + "manifestVersion": "0.1" +} diff --git a/packages/snaps-cli/src/commands/watch/watch.e2e.test.ts b/packages/snaps-cli/src/commands/watch/watch.e2e.test.ts index 5ed539fed8..a5259f3796 100644 --- a/packages/snaps-cli/src/commands/watch/watch.e2e.test.ts +++ b/packages/snaps-cli/src/commands/watch/watch.e2e.test.ts @@ -64,4 +64,46 @@ describe('mm-snap watch', () => { ); }, ); + + it('evaluates a bundle and reports manifest warnings', async () => { + runner = getCommandRunner( + 'watch', + ['--port', '0'], + resolve(__dirname, '__test__', 'invalid'), + ); + + await runner.waitForStderr( + /Compiled \d+ files? in \d+ms with \d+ warnings?\./u, + ); + + expect(runner.stdout).toContainEqual( + expect.stringMatching(/Checking the input file\./u), + ); + expect(runner.stdout).toContainEqual( + expect.stringMatching(/Starting the development server\./u), + ); + expect(runner.stdout).toContainEqual( + expect.stringMatching( + /The server is listening on http:\/\/localhost:\d+\./u, + ), + ); + expect(runner.stdout).toContainEqual( + expect.stringMatching(/Building the Snap bundle\./u), + ); + expect(runner.stderr).toContainEqual( + expect.stringMatching( + /Compiled \d+ files? in \d+ms with \d+ warnings?\./u, + ), + ); + expect(runner.stderr).toContainEqual( + expect.stringContaining( + 'No icon found in the Snap manifest. It is recommended to include an icon for the Snap. See https://docs.metamask.io/snaps/how-to/design-a-snap/#guidelines-at-a-glance for more information.', + ), + ); + expect(runner.stderr).toContainEqual( + expect.stringContaining( + 'The Snap exports the following handlers, but does not request permission for them: onRpcRequest (endowment:rpc).', + ), + ); + }); }); diff --git a/packages/snaps-cli/src/webpack/__snapshots__/config.test.ts.snap b/packages/snaps-cli/src/webpack/__snapshots__/config.test.ts.snap index 796bf82972..34d7377870 100644 --- a/packages/snaps-cli/src/webpack/__snapshots__/config.test.ts.snap +++ b/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", ], diff --git a/packages/snaps-cli/src/webpack/config.ts b/packages/snaps-cli/src/webpack/config.ts index 502cd70838..2652edcb98 100644 --- a/packages/snaps-cli/src/webpack/config.ts +++ b/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, diff --git a/packages/snaps-cli/src/webpack/plugins.test.ts b/packages/snaps-cli/src/webpack/plugins.test.ts index 08873f45df..d25159d9ec 100644 --- a/packages/snaps-cli/src/webpack/plugins.test.ts +++ b/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((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((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', () => { diff --git a/packages/snaps-cli/src/webpack/plugins.ts b/packages/snaps-cli/src/webpack/plugins.ts index b1ebdbe746..b07eeecee7 100644 --- a/packages/snaps-cli/src/webpack/plugins.ts +++ b/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); - } - } } /** diff --git a/packages/snaps-utils/src/manifest/manifest.ts b/packages/snaps-utils/src/manifest/manifest.ts index a5724df49f..7fb896103d 100644 --- a/packages/snaps-utils/src/manifest/manifest.ts +++ b/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; + + /** + * 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; * 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 { 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 { 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( diff --git a/packages/snaps-utils/src/manifest/validator.test.ts b/packages/snaps-utils/src/manifest/validator.test.ts new file mode 100644 index 0000000000..37211fdadf --- /dev/null +++ b/packages/snaps-utils/src/manifest/validator.test.ts @@ -0,0 +1,70 @@ +import { isReportFixable } from './validator'; +import type { ValidatorReport } from './validator-types'; +import { getSnapManifest } from '../test-utils'; + +describe('isReportFixable', () => { + it('returns `true` if the error report has a fix', () => { + const report = { + severity: 'error', + message: 'foo', + fix: async () => { + return { + manifest: getSnapManifest(), + }; + }, + } satisfies ValidatorReport; + + expect(isReportFixable(report)).toBe(true); + }); + + it('returns `true` if the warning report has a fix', () => { + const report = { + severity: 'warning', + message: 'foo', + fix: async () => { + return { + manifest: getSnapManifest(), + }; + }, + } satisfies ValidatorReport; + + expect(isReportFixable(report)).toBe(true); + }); + + it('returns `false` if the report has no fix', () => { + const report = { + severity: 'error', + message: 'foo', + } satisfies ValidatorReport; + + expect(isReportFixable(report)).toBe(false); + }); + + it('returns `false` if the error report has a fix, and `errorsOnly` is `true`', () => { + const report = { + severity: 'error', + message: 'foo', + fix: async () => { + return { + manifest: getSnapManifest(), + }; + }, + } satisfies ValidatorReport; + + expect(isReportFixable(report, true)).toBe(true); + }); + + it('returns `false` if the warning report has a fix, and `errorsOnly` is `true`', () => { + const report = { + severity: 'warning', + message: 'foo', + fix: async () => { + return { + manifest: getSnapManifest(), + }; + }, + } satisfies ValidatorReport; + + expect(isReportFixable(report, true)).toBe(false); + }); +}); diff --git a/packages/snaps-utils/src/manifest/validator.ts b/packages/snaps-utils/src/manifest/validator.ts index d5dcdc9051..a053e740e2 100644 --- a/packages/snaps-utils/src/manifest/validator.ts +++ b/packages/snaps-utils/src/manifest/validator.ts @@ -102,12 +102,27 @@ export async function runValidators( }; } +/** + * Check whether a report is fixable. + * + * @param report - The report to check. + * @param errorsOnly - Whether to only consider errors for fixability. + * @returns Whether the report is fixable. + */ +export function isReportFixable(report: ValidatorReport, errorsOnly?: boolean) { + return Boolean(report.fix && (!errorsOnly || report.severity === 'error')); +} + /** * Get whether any reports has pending fixes. * * @param results - Results of the validation run. + * @param errorsOnly - Whether to only consider errors for pending fixes. * @returns Whether there are fixes pending. */ -export function hasFixes(results: ValidatorResults): boolean { - return results.reports.some((report) => report.fix); +export function hasFixes( + results: ValidatorResults, + errorsOnly?: boolean, +): boolean { + return results.reports.some((report) => isReportFixable(report, errorsOnly)); } diff --git a/packages/snaps-webpack-plugin/src/plugin.test.ts b/packages/snaps-webpack-plugin/src/plugin.test.ts index 77f40b7ce0..3062638e1c 100644 --- a/packages/snaps-webpack-plugin/src/plugin.test.ts +++ b/packages/snaps-webpack-plugin/src/plugin.test.ts @@ -12,7 +12,8 @@ import { DEFAULT_SNAP_BUNDLE } from '@metamask/snaps-utils/test-utils'; import type { IFs } from 'memfs'; import { createFsFromVolume, Volume } from 'memfs'; import type { IPromisesAPI } from 'memfs/lib/promises'; -import type { Stats, Configuration } from 'webpack'; +import { promisify } from 'util'; +import type { Stats, Configuration, Watching } from 'webpack'; // TODO: Either fix this lint violation or explain why it's necessary to // ignore. // eslint-disable-next-line import-x/no-named-as-default @@ -87,6 +88,55 @@ const bundle = async ({ }; }; +const watch = async ({ + code = DEFAULT_SNAP_BUNDLE, + options = { eval: false, manifestPath: undefined }, + fileSystem = createFsFromVolume(new Volume()), + webpackOptions, +}: BundleOptions = {}): Promise<{ + code: string; + fs: IPromisesAPI; + watching: Watching; +}> => { + const { promises: fs } = fileSystem; + + const bundler = webpack({ + watch: true, + mode: 'none', + entry: { + foo: '/foo.js', + }, + output: { + path: '/lib', + filename: '[name].js', + }, + plugins: [new SnapsWebpackPlugin(options)], + ...webpackOptions, + }); + + bundler.inputFileSystem = fileSystem; + bundler.outputFileSystem = fileSystem; + + await fs.mkdir('/lib', { recursive: true }); + await fs.writeFile('/foo.js', code); + + const outputWatching = await new Promise((resolve, reject) => { + const watching = bundler.watch({}, (error, stats) => { + if (error || !stats) { + return reject(error); + } + + return resolve(watching); + }); + }); + + return { + code: (await fs.readFile('/lib/foo.js', 'utf-8')) as string, + fs, + watching: outputWatching, + }; +}; + describe('SnapsWebpackPlugin', () => { it('processes files using Webpack', async () => { const { code } = await bundle(); @@ -232,6 +282,7 @@ describe('SnapsWebpackPlugin', () => { updateAndWriteManifest: true, sourceCode: expect.any(String), writeFileFn: expect.any(Function), + watchMode: false, }); const writeFileFn = mock.mock.calls[0][1]?.writeFileFn; @@ -275,6 +326,7 @@ describe('SnapsWebpackPlugin', () => { updateAndWriteManifest: true, sourceCode: expect.any(String), writeFileFn: expect.any(Function), + watchMode: false, }); }); @@ -301,7 +353,41 @@ describe('SnapsWebpackPlugin', () => { updateAndWriteManifest: false, sourceCode: expect.any(String), writeFileFn: expect.any(Function), + watchMode: false, + }); + }); + + it('does not fix warnings in the manifest if the compiler is in watch mode', async () => { + const mock = checkManifest as jest.MockedFunction; + mock.mockResolvedValue({ + files: undefined, + updated: false, + reports: [], + }); + + const { watching } = await watch({ + options: { + eval: false, + manifestPath: '/snap.manifest.json', + writeManifest: false, + }, + webpackOptions: { + watch: true, + }, + }); + + expect(mock).toHaveBeenCalledTimes(1); + expect(mock).toHaveBeenCalledWith('/', { + exports: undefined, + handlerEndowments, + updateAndWriteManifest: false, + sourceCode: expect.any(String), + writeFileFn: expect.any(Function), + watchMode: true, }); + + const close = promisify(watching.close.bind(watching)); + await close(); }); it('logs manifest errors if writeManifest is disabled', async () => { diff --git a/packages/snaps-webpack-plugin/src/plugin.ts b/packages/snaps-webpack-plugin/src/plugin.ts index 582b18d406..0f689c724e 100644 --- a/packages/snaps-webpack-plugin/src/plugin.ts +++ b/packages/snaps-webpack-plugin/src/plugin.ts @@ -194,6 +194,7 @@ export default class SnapsWebpackPlugin { sourceCode: bundleContent, exports, handlerEndowments, + watchMode: compiler.watchMode, writeFileFn: async (path, data) => { assert( compiler.outputFileSystem,