Skip to content

Navigation Menu

Sign in
Sign up

metro-file-map: Vendor walker and remove the dependency - #1905

Closed
robhogan wants to merge 1 commit into
main from
pr1905
Closed

metro-file-map: Vendor walker and remove the dependency #1905
robhogan wants to merge 1 commit into
main from
pr1905

Conversation

@robhogan

@robhogan robhogan commented Sep 6, 2026
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

metro-file-map depends on walker for FallbackWatcher's initial crawl only. It hasn't received updates since 2021 and pulls in two other dependencies, yet is only ~120 lines. For that alone, vendoring is preferable to the supply chain risk.

There's also a known Metro bug we can't work around with walker as-is: directory events only fire after walker has statted their contents, so we can't attach a watch to the directory until after a short delay. Files added between the initial enumeration and the watch being attached are missed - and this is common when the directory is immediately populated eg by a package installation. This PR doesn't fix that, but it unblocks the follow up.

This is a mostly verbatim port that just adds Flow typing, ESM syntax, and updates an import from fs to Metro-convention node:fs.

Changelog: [Internal] Vendor the walker dependency into metro-file-map

Test plan

yarn flow check
yarn typecheck-ts
yarn lint
yarn verify-api-snapshots
yarn build
yarn jest

All clean - full Jest run is 2662 passed, 1 skipped, across 147 suites.

recReaddir drives FallbackWatcher's initial crawl, so packages/metro-file-map/src/watchers/__tests__/integration-test.js exercises the vendored code directly - 34 tests, including the Fallback matrix over new/changed/deleted files, symlinks (including to non-existent targets), pre-existing trees moved in and out of the watched root, and directory deletion. There's no dedicated unit test for walker.js itself.

One caveat: the first full run failed at integration-test.js:92, on the NativeWatcher directory-event assertion rather than Fallback. It passed in isolation and on a clean re-run of the whole suite, so it reads as a timing flake under parallel load, but I haven't bisected it against unmodified main to prove that.

## Summary
`metro-file-map` depends on `walker` for `FallbackWatcher`'s initial crawl and for nothing else - a single call site in `recReaddir`. It's ~120 lines of recursive `lstat`/`readdir`, unmaintained since 2021, untyped (we carry a `$FlowFixMe[untyped-import]` for it), and it pulls in `makeerror` and `tmpl` purely to construct one error class we never inspect.
This diff brings it in-tree at `packages/metro-file-map/src/third-party/walker.js`, following the `buck-worker-tool/src/third-party` convention - the Apache-2.0 notice retained as `LICENSE.APACHE2` alongside it, and a `Portions (c) Meta Platforms` header over Metro's own MIT notice. Behaviour is preserved: the same event set (`entry` followed by a type-specific event, plus `error` and `end`), the same `filterDir` semantics, the same pending-counter completion, and the same `lstat` dispatch order - which matters, because `isFile()` is tested last and only after the device types. It's modernised where Flow required it: an ES class composing an `EventEmitter` rather than `util.inherits`, `node:fs`/`node:path`, and a local `UnknownFileTypeError` subclass in place of `makeerror`.
`on()` is typed against a keyed listener map, so both the event name and the listener signature are checked at the call site. I checked that isn't vacuous - renaming `'dir'` to `'dirr'` in `recReaddir` gives `property 'dirr' (did you mean 'dir'?) is missing in EventListeners`.
Two consequences worth calling out:
 - Because `walker` required the unprefixed `'fs'`, `resolver-test.js` carried a `jest.mock('fs', ...)` aliasing it to the same in-memory fs as `node:fs`. The vendored copy uses `node:fs`, so that goes.
 - Apache-2.0 source is now in-tree in an MIT repo. It was already in the dependency graph and it ships either way, but this puts it in the tarball as our own source, so it's worth a look from that angle. `LICENSE.APACHE2` is copied to `build/third-party/` by `scripts/build.js`, which copies non-JS files under `src/` verbatim.
The file is under `third-party/` rather than `vendor/` deliberately. Both are in `.eslintignore`, but only `packages/**/third-party` is in `IGNORED_PATTERNS` in `generateTypeScriptDefinitions.js` - under `vendor/` the build fails at `generateTypeScriptDefinitions.js:164` reading `lintResult.output`, because ESLint returns no result at all for an ignored file. So `third-party/` is the name both configs already agree on, and this way needs no config change.
`walker` stays in `yarn.lock` under `jest-haste-map`, which is dev-only, so no published Metro package depends on it (or on `makeerror`/`tmpl`) any more.
Changelog: [Internal] Vendor the `walker` dependency into `metro-file-map`
## Test plan
```
yarn flow check
yarn typecheck-ts
yarn lint
yarn verify-api-snapshots
yarn build
yarn jest
```
All clean - full Jest run is 2662 passed, 1 skipped, across 147 suites.
`recReaddir` drives `FallbackWatcher`'s initial crawl, so `packages/metro-file-map/src/watchers/__tests__/integration-test.js` exercises the vendored code directly - 34 tests, including the `Fallback` matrix over new/changed/deleted files, symlinks (including to non-existent targets), pre-existing trees moved in and out of the watched root, and directory deletion. There's no dedicated unit test for `walker.js` itself.
One caveat: the first full run failed at `integration-test.js:92`, on the `NativeWatcher` directory-event assertion rather than `Fallback`. It passed in isolation and on a clean re-run of the whole suite, so it reads as a timing flake under parallel load, but I haven't bisected it against unmodified main to prove that.
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 6, 2026

robhogan commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

Abandoning in favour of #1906

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

AltStyle によって変換されたページ (->オリジナル) /