From d42120cfebc1fe3559be05619faacb21d63f8f76 Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Fri, 25 Sep 2026 19:03:16 +0100 Subject: [PATCH] metro-file-map: Let plugins report what changed in a batch, on the change event Stacked on https://github.com/react/metro/pull/1952. ## Summary A file map plugin often knows things about a batch of changes that nothing else can work out. The motivating case is Haste - https://github.com/react/metro/pull/1952 records the Haste names a resolution looks up, and to invalidate from those a consumer needs to know which names were rebound by a change. It can't derive that from the `ChangeEvent`, which carries paths and modified times, and by the time it sees the event a removed file's name is already gone from the map. `HastePlugin` has it to hand - its `onChanged` is given each removed file's old name and each added file's new one. This gives plugins a way to say so. `FileMapPlugin.onChanged` may now return a summary, and the file map publishes what each plugin returned as `ChangeEvent.pluginChanges`, by plugin name. A plugin that returns nothing is absent, so existing plugins are unaffected - the summary is a third type parameter on `FileMapPlugin` that defaults to `void`. `getPluginChanges(event, plugin)` reads an entry with the plugin's own summary type, so a consumer never writes the name or casts. The summary describes exactly the batch in `changes`. Plugins are already updated synchronously, immediately before the emit, so there's no ordering for a consumer to get wrong - which is the problem with the alternative of asking the plugin afterwards. There are two emit sites: the batched watch path updates plugins inline, and a recrawl updates them inside `#applyFileDelta`, which now returns what they reported along with the changes. The initial build goes through `#applyFileDelta` too but emits nothing, so its summaries go unused. `HastePlugin` is the first user. It reports `changedNames`, the names bound or unbound for any platform by the files added and removed - a bare name, matching what #1952 records. Plugin names now have to be unique across all of a file map's plugins. That was only checked for plugins with a worker, but persisted plugin state is keyed by name for every plugin, so two that shared a name would already have overwritten each other's state. Two things worth knowing: - On a cold start `onChanged` sees every file as added, so `HastePlugin` builds a set of every name for a summary nobody reads. That's nothing where Haste is inert, which is Metro's default, and transient otherwise. If it matters, the alternative is an optional method the file map calls only for a batch it's about to emit. - A modified file reaches plugins in `modifiedFiles` with its new data only, and `HastePlugin.onChanged` doesn't read that list. So an edit that changes a file's Haste name (a docblock, or a global package's `name`) is neither applied to the map nor reported here. That predates this, and can't happen with a path-based `hasteImpl`. `FileSystemChangeAggregator` keeps each file's pre-batch metadata, so giving plugins the old value is feasible, separately. Nothing consumes this yet. ## Test plan ``` yarn jest packages/metro-file-map packages/metro-resolver packages/metro/src/node-haste packages/metro/src/DeltaBundler packages/metro/src/integration_tests packages/metro/src/__tests__ packages/metro/src/Server yarn flow check yarn typecheck-ts yarn verify-api-snapshots ``` - `HastePlugin-test.js`: `onChanged` reports the names bound and unbound, and nothing when no file with a name was added or removed. - `index-test.js`: in watch mode an added and a removed file put both names on the event, read back through `getPluginChanges`, and plugins with nothing to say are absent. A recrawl publishes what plugins reported too. Two plugins sharing a name throw, with or without a worker. Changelog: Internal --- packages/metro-file-map/API.md | 31 ++++++-- .../src/__tests__/index-test.js | 56 ++++++++++++-- packages/metro-file-map/src/flow-types.js | 20 ++++- packages/metro-file-map/src/index.js | 77 ++++++++++++------- .../src/lib/getPluginChanges.js | 24 ++++++ .../metro-file-map/src/plugins/HastePlugin.js | 29 ++++++- .../haste/__tests__/HastePlugin-test.js | 34 +++++++- 7 files changed, 229 insertions(+), 42 deletions(-) create mode 100644 packages/metro-file-map/src/lib/getPluginChanges.js diff --git a/packages/metro-file-map/API.md b/packages/metro-file-map/API.md index 6e69a8edc1..6d9887f4a6 100644 --- a/packages/metro-file-map/API.md +++ b/packages/metro-file-map/API.md @@ -47,6 +47,7 @@ export type CacheManagerWriteOptions = Readonly<{ export type ChangeEvent = Readonly<{ logger?: null | undefined | RootPerfLogger; changes: ReadonlyFileSystemChanges>; + pluginChanges: ReadonlyMap; rootDir: string; }>; @@ -191,6 +192,25 @@ interface FileSystem_2 { } export {FileSystem_2 as FileSystem}; +export function getPluginChanges( + event: ChangeEvent, + plugin: FileMapPlugin< + /** + * > | plugin: FileMapPlugin, + * | ^^^^^ Unsupported feature: Translating "empty type" is currently not supported. + **/ + any, + /** + * > | plugin: FileMapPlugin, + * | ^^^^^ Unsupported feature: Translating "empty type" is currently not supported. + **/ + any, + ChangeSummary + >, +): ChangeSummary | void; + +export type HasteChanges = Readonly<{changedNames: ReadonlySet}>; + export class HasteConflictsError extends Error { constructor(conflicts: ReadonlyArray); getDetailedMessage(pathsRelativeToRoot: null | undefined | string): string; @@ -209,7 +229,7 @@ export type HasteMapItem = { [platform: string]: HasteMapItemMetadata; }; -export class HastePlugin implements HasteMap, FileMapPlugin { +export class HastePlugin implements HasteMap, FileMapPlugin { constructor(options: HasteMapOptions); assertValid(): void; computeConflicts(): Array; @@ -221,7 +241,7 @@ export class HastePlugin implements HasteMap, FileMapPlugin getWorker(): FileMapPluginWorker; initialize(opts: FileMapPluginInitOptions): Promise; readonly name: 'haste'; - onChanged(delta: ReadonlyFileSystemChanges): void; + onChanged(delta: ReadonlyFileSystemChanges): HasteChanges | void; setModule(id: string, module: HasteMapItemMetadata): void; } @@ -247,15 +267,16 @@ export type HealthCheckResult = export type InputFileMapPlugin = FileMapPlugin< /** - * > | export type InputFileMapPlugin = FileMapPlugin; + * > | export type InputFileMapPlugin = FileMapPlugin; * | ^^^^^ Unsupported feature: Translating "empty type" is currently not supported. **/ any, /** - * > | export type InputFileMapPlugin = FileMapPlugin; + * > | export type InputFileMapPlugin = FileMapPlugin; * | ^^^^^ Unsupported feature: Translating "empty type" is currently not supported. **/ - any + any, + unknown >; export type InputOptions = Readonly<{ diff --git a/packages/metro-file-map/src/__tests__/index-test.js b/packages/metro-file-map/src/__tests__/index-test.js index 9ac6da3611..7222e49243 100644 --- a/packages/metro-file-map/src/__tests__/index-test.js +++ b/packages/metro-file-map/src/__tests__/index-test.js @@ -27,7 +27,7 @@ import type { } from '../flow-types'; import type {DependencyPlugin} from '../index'; import type {default as FileMapT} from '../index'; -import type {HasteMapOptions} from '../plugins/HastePlugin'; +import type HastePluginT, {HasteMapOptions} from '../plugins/HastePlugin'; import type {MockMapOptions} from '../plugins/MockPlugin'; import typeof WorkerModule from '../worker'; @@ -292,7 +292,7 @@ let buildNewFileMap: ( ) => Promise<{ ...BuildResult, fileMap: FileMapT, - hasteMap: HasteMap, + hasteMap: HastePluginT, mockMap: ?MockMap, dependencyPlugin: ?DependencyPlugin, }>; @@ -445,6 +445,24 @@ describe('FileMap', () => { expect(FileMap.H).toBe(require('../constants').default); }); + test('throws on plugins that share a name, with or without a worker', () => { + const MockPlugin = require('../plugins/MockPlugin').default; + const mockConfig = { + console: globalThis.console, + rootDir: defaultConfig.rootDir, + mocksPattern: /__mocks__/, + throwOnModuleCollision: false, + }; + expect( + () => + new FileMap({ + ...defaultConfig, + // MockPlugin has no worker, so holds no plugin data slot + plugins: [new MockPlugin(mockConfig), new MockPlugin(mockConfig)], + }), + ).toThrow('metro-file-map: Duplicate plugin name: mocks'); + }); + test('ignores files given a pattern', async () => { mockFs[p('/project/fruits/Kiwi.js')] = ` // Kiwi! @@ -1683,6 +1701,28 @@ describe('FileMap', () => { } }); + fm_it( + 'publishes what each plugin reports about a batch on the change event', + async ({fileMap, hasteMap}) => { + const getPluginChanges = require('../lib/getPluginChanges').default; + const fruitsRoot = path.join('/', 'project', 'fruits'); + mockFs[path.join(fruitsRoot, 'Tomato.js')] = '// Tomato!'; + mockEmitters[fruitsRoot].emitFileEvent({ + event: 'touch', + relativePath: 'Tomato.js', + metadata: MOCK_CHANGE_FILE, + }); + mockDeleteFile(fruitsRoot, 'Banana.js'); + const changeEvent = await waitForItToChange(fileMap); + + expect(getPluginChanges(changeEvent, hasteMap)).toEqual({ + changedNames: new Set(['Tomato', 'Banana']), + }); + // Only plugins with something to say are present + expect([...changeEvent.pluginChanges.keys()]).toEqual([hasteMap.name]); + }, + ); + function mockDeleteFile(root: string, relativePath: string) { const e = mockEmitters[root]; e.emitFileEvent({event: 'delete', relativePath}); @@ -1697,7 +1737,7 @@ describe('FileMap', () => { function fm_it( title: string, - fn: (fm: Readonly<{fileMap: FileMap, hasteMap: HasteMap}>) => unknown, + fn: (fm: Readonly<{fileMap: FileMap, hasteMap: HastePluginT}>) => unknown, options?: FileMapTestOptions = {}, ): void { options = options || {}; @@ -1720,7 +1760,7 @@ describe('FileMap', () => { fm_it.only = ( title: string, - fn: (fm: Readonly<{fileMap: FileMap, hasteMap: HasteMap}>) => unknown, + fn: (fm: Readonly<{fileMap: FileMap, hasteMap: HastePluginT}>) => unknown, options?: FileMapTestOptions, ): void => fm_it(title, fn, {...options, only: true}); @@ -2510,7 +2550,13 @@ describe('FileMap', () => { relativePath: 'tropical', }); - await waitForItToChange(fileMap); + const changeEvent = await waitForItToChange(fileMap); + + // Plugins are updated inside the recrawl, and what they report + // still reaches the event + expect( + require('../lib/getPluginChanges').default(changeEvent, hasteMap), + ).toEqual({changedNames: new Set(['Mango', 'Papaya'])}); // Verify crawl was called with the correct directory expect(mockNodeCrawler).toHaveBeenNthCalledWith( diff --git a/packages/metro-file-map/src/flow-types.js b/packages/metro-file-map/src/flow-types.js index 802c1bf429..0124851f04 100644 --- a/packages/metro-file-map/src/flow-types.js +++ b/packages/metro-file-map/src/flow-types.js @@ -111,6 +111,12 @@ export type ChangedFileMetadata = Readonly<{ export type ChangeEvent = Readonly<{ logger?: ?RootPerfLogger, changes: ReadonlyFileSystemChanges>, + /** + * What each plugin reported about this batch of changes, by plugin name. A + * plugin whose `onChanged` returned nothing is absent. Read an entry with + * `getPluginChanges`, which knows its type. + */ + pluginChanges: ReadonlyMap, rootDir: string, }>; @@ -278,19 +284,29 @@ export type V8Serializable = V8SerializablePrimitive | V8SerializableCollection; export interface FileMapPlugin< in SerializableState extends void | V8Serializable = void | V8Serializable, in PerFileData extends void | V8Serializable = void | V8Serializable, + out ChangeSummary = void, > { + // Must be unique among the plugins of a file map. readonly name: string; initialize( initOptions: FileMapPluginInitOptions, ): Promise; assertValid(): void; - onChanged(changes: ReadonlyFileSystemChanges): void; + /** + * Update the plugin's state for a batch of changes. It is called + * synchronously, immediately before the batch is emitted as a `ChangeEvent`. + * + * A plugin may return a summary of what changed in terms only it knows, + * such as the Haste names that were rebound, for the file map to publish as + * `ChangeEvent.pluginChanges`. Return nothing if there is nothing to say. + */ + onChanged(changes: ReadonlyFileSystemChanges): ChangeSummary; getSerializableSnapshot(): void | V8Serializable; getCacheKey(): string; getWorker(): ?FileMapPluginWorker; } -export type InputFileMapPlugin = FileMapPlugin; +export type InputFileMapPlugin = FileMapPlugin; export interface MetadataWorker { processFile( diff --git a/packages/metro-file-map/src/index.js b/packages/metro-file-map/src/index.js index e2a56abd43..2d032845b0 100644 --- a/packages/metro-file-map/src/index.js +++ b/packages/metro-file-map/src/index.js @@ -123,7 +123,7 @@ type InternalOptions = Readonly<{ type IndexedPlugin = Readonly<{ // $FlowFixMe[unclear-type] Plugin types cannot be known statically - plugin: FileMapPlugin, + plugin: FileMapPlugin, dataIdx: ?number, }>; type InternalEnqueuedEvent = Readonly< @@ -148,7 +148,9 @@ export type {FileDataPluginOptions} from './plugins/FileDataPlugin'; export type {DependencyPluginOptions} from './plugins/DependencyPlugin'; export {DuplicateHasteCandidatesError} from './plugins/haste/DuplicateHasteCandidatesError'; export {HasteConflictsError} from './plugins/haste/HasteConflictsError'; +export {default as getPluginChanges} from './lib/getPluginChanges'; export {default as HastePlugin} from './plugins/HastePlugin'; +export type {HasteChanges} from './plugins/HastePlugin'; export type {HasteMap} from './flow-types'; export type {HealthCheckResult} from './Watcher'; @@ -321,19 +323,21 @@ export default class FileMap extends EventEmitter { const pluginWorkers: Array = []; const pluginDataIndices = new Map(); const plugins = options.plugins ?? []; + const pluginNames = new Set(); for (const plugin of plugins) { + // Plugin data slots, persisted plugin state and change summaries are all + // addressed by name, so a second plugin of the same name would silently + // take over the first's. + invariant( + !pluginNames.has(plugin.name), + 'metro-file-map: Duplicate plugin name: %s', + plugin.name, + ); + pluginNames.add(plugin.name); const maybeWorker = plugin.getWorker(); const dataIdx = maybeWorker != null ? dataSlot++ : null; indexedPlugins.push({plugin, dataIdx}); if (dataIdx != null) { - // Crawlers address plugin data by name, so names must be unique among - // plugins holding a slot - otherwise a crawler would silently write to - // the shadowed plugin's slot. - invariant( - !pluginDataIndices.has(plugin.name), - 'metro-file-map: Duplicate plugin name: %s', - plugin.name, - ); pluginDataIndices.set(plugin.name, dataIdx); } if (maybeWorker != null) { @@ -515,7 +519,9 @@ export default class FileMap extends EventEmitter { ]); // Update `fileSystem` and plugins based on the file delta. - const actualChanges = await this.#applyFileDelta( + // Nothing is emitted for the initial build, so what plugins say about + // it goes unused. + const {changeAggregator: actualChanges} = await this.#applyFileDelta( fileSystem, plugins, fileDelta, @@ -645,6 +651,27 @@ export default class FileMap extends EventEmitter { return null; } + /** + * Bring every plugin up to date with a batch of changes, and collect what + * each has to say about it for `ChangeEvent.pluginChanges`. + */ + #updatePlugins( + changeAggregator: FileSystemChangeAggregator, + ): Map { + const pluginChanges = new Map(); + this.#plugins.forEach(({plugin, dataIdx}) => { + const summary = plugin.onChanged( + changeAggregator.getMappedView( + dataIdx != null ? metadata => metadata[dataIdx] : () => null, + ), + ); + if (summary != null) { + pluginChanges.set(plugin.name, summary); + } + }); + return pluginChanges; + } + async #applyFileDelta( fileSystem: MutableFileSystem, plugins: ReadonlyArray, @@ -653,7 +680,10 @@ export default class FileMap extends EventEmitter { removedFiles: ReadonlySet, clocks?: WatchmanClocks, }>, - ): Promise { + ): Promise<{ + changeAggregator: FileSystemChangeAggregator, + pluginChanges: ReadonlyMap, + }> { this.#startupPerfLogger?.point('applyFileDelta_start'); const {changedFiles, removedFiles} = delta; this.#startupPerfLogger?.point('applyFileDelta_preprocess_start'); @@ -742,17 +772,11 @@ export default class FileMap extends EventEmitter { this.#startupPerfLogger?.point('applyFileDelta_add_end'); this.#startupPerfLogger?.point('applyFileDelta_updatePlugins_start'); - this.#plugins.forEach(({plugin, dataIdx}) => { - plugin.onChanged( - changeAggregator.getMappedView( - dataIdx != null ? metadata => metadata[dataIdx] : () => null, - ), - ); - }); + const pluginChanges = this.#updatePlugins(changeAggregator); this.#startupPerfLogger?.point('applyFileDelta_updatePlugins_end'); this.#startupPerfLogger?.point('applyFileDelta_end'); - return changeAggregator; + return {changeAggregator, pluginChanges}; } /** @@ -860,13 +884,7 @@ export default class FileMap extends EventEmitter { } const _netChange = changeAggregator.getView(); - this.#plugins.forEach(({plugin, dataIdx}) => { - plugin.onChanged( - changeAggregator.getMappedView( - dataIdx != null ? metadata => metadata[dataIdx] : () => null, - ), - ); - }); + const pluginChanges = this.#updatePlugins(changeAggregator); const toPublicMetadata = ( metadata: Readonly, @@ -893,6 +911,7 @@ export default class FileMap extends EventEmitter { const changeEvent: ChangeEvent = { changes: changesWithMetadata, logger: hmrPerfLogger, + pluginChanges, rootDir: this.#options.rootDir, }; this.emit('change', changeEvent); @@ -1039,7 +1058,10 @@ export default class FileMap extends EventEmitter { } // Reuse the same batch processing logic as build() - const recrawlChangeAggregator = await this.#applyFileDelta( + const { + changeAggregator: recrawlChangeAggregator, + pluginChanges: recrawlPluginChanges, + } = await this.#applyFileDelta( fileSystem, this.#plugins, crawlResult, @@ -1067,6 +1089,7 @@ export default class FileMap extends EventEmitter { const changeEvent: ChangeEvent = { changes: changesWithMetadata, logger: null, + pluginChanges: recrawlPluginChanges, rootDir: this.#options.rootDir, }; this.emit('change', changeEvent); diff --git a/packages/metro-file-map/src/lib/getPluginChanges.js b/packages/metro-file-map/src/lib/getPluginChanges.js new file mode 100644 index 0000000000..f1ca9821a0 --- /dev/null +++ b/packages/metro-file-map/src/lib/getPluginChanges.js @@ -0,0 +1,24 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow strict-local + * @format + * @oncall react_native + */ + +import type {ChangeEvent, FileMapPlugin} from '../flow-types'; + +/** + * What `plugin` reported about the batch of changes in `event`, as returned + * by its `onChanged`, or undefined if it reported nothing. + */ +export default function getPluginChanges( + event: ChangeEvent, + plugin: FileMapPlugin, +): ChangeSummary | void { + // $FlowFixMe[incompatible-type] Only `plugin` writes the entry under its name + return event.pluginChanges.get(plugin.name); +} diff --git a/packages/metro-file-map/src/plugins/HastePlugin.js b/packages/metro-file-map/src/plugins/HastePlugin.js index 9827ba6be6..d3598d885f 100644 --- a/packages/metro-file-map/src/plugins/HastePlugin.js +++ b/packages/metro-file-map/src/plugins/HastePlugin.js @@ -52,8 +52,19 @@ export type HasteMapOptions = Readonly<{ failValidationOnConflicts: boolean, }>; +/** + * What `HastePlugin` reports about a batch of changes, as + * `getPluginChanges(changeEvent, hastePlugin)`. + */ +export type HasteChanges = Readonly<{ + // Names of modules and packages that were bound or unbound for any platform + // by the files added and removed. The result of looking one of these up may + // have changed, including from or to a duplicate. + changedNames: ReadonlySet, +}>; + export default class HastePlugin - implements HasteMap, FileMapPlugin + implements HasteMap, FileMapPlugin { readonly name: 'haste' = 'haste'; @@ -236,14 +247,28 @@ export default class HastePlugin ); } - onChanged(delta: ReadonlyFileSystemChanges): void { + onChanged(delta: ReadonlyFileSystemChanges): HasteChanges | void { + let changedNames: ?Set = null; + const onNameChanged = (name: string) => { + if (changedNames == null) { + changedNames = new Set(); + } + changedNames.add(name); + }; // Process removals first so that moves aren't treated as duplicates. for (const [canonicalPath, maybeHasteId] of delta.removedFiles) { this.#onRemovedFile(canonicalPath, maybeHasteId); + if (maybeHasteId != null) { + onNameChanged(maybeHasteId); + } } for (const [canonicalPath, maybeHasteId] of delta.addedFiles) { this.#onNewFile(canonicalPath, maybeHasteId); + if (maybeHasteId != null) { + onNameChanged(maybeHasteId); + } } + return changedNames != null ? {changedNames} : undefined; } #onNewFile(canonicalPath: string, id: ?string) { diff --git a/packages/metro-file-map/src/plugins/haste/__tests__/HastePlugin-test.js b/packages/metro-file-map/src/plugins/haste/__tests__/HastePlugin-test.js index 59adc7c55d..b694f6c0ff 100644 --- a/packages/metro-file-map/src/plugins/haste/__tests__/HastePlugin-test.js +++ b/packages/metro-file-map/src/plugins/haste/__tests__/HastePlugin-test.js @@ -101,10 +101,11 @@ describe.each([['win32'], ['posix']])('HastePlugin on %s', platform => { beforeEach(async () => { hasteMap = new HasteMap(opts); - removeFile = (canonicalPath, name) => + removeFile = (canonicalPath, name) => { hasteMap.onChanged( makeChanges({added: [], removed: [[canonicalPath, name]]}), ); + }; await hasteMap.initialize({ files: { fileIterator: jest.fn().mockReturnValue(INITIAL_FILES), @@ -177,6 +178,37 @@ describe.each([['win32'], ['posix']])('HastePlugin on %s', platform => { DuplicateHasteCandidatesError, ); }); + + test('reports the names that were bound and unbound', () => { + expect( + hasteMap.onChanged( + makeChanges({ + added: [ + [p('project/Baz.js'), 'Baz'], + [p('project/other/Bar.js'), 'Bar'], + [p('project/NotHaste.js'), null], + ], + removed: [ + [p('project/Duplicate.js'), 'Duplicate'], + [p('project/Foo.js'), 'NameForFoo'], + ], + }), + ), + ).toEqual({ + changedNames: new Set(['Baz', 'Bar', 'Duplicate', 'NameForFoo']), + }); + }); + + test('reports nothing when no file with a name was added or removed', () => { + expect( + hasteMap.onChanged( + makeChanges({ + added: [[p('project/NotHaste.js'), null]], + removed: [[p('project/AlsoNotHaste.js'), null]], + }), + ), + ).toBeUndefined(); + }); }); describe('getModuleNameByPath', () => {