Skip to content

feat: add support for capturing require.context in dependency collector#821

Closed
EvanBacon wants to merge 29 commits into
react:mainfrom
EvanBacon:@evanbacon/require-context/collect
Closed

feat: add support for capturing require.context in dependency collector#821
EvanBacon wants to merge 29 commits into
react:mainfrom
EvanBacon:@evanbacon/require-context/collect

Conversation

@EvanBacon

@EvanBacon EvanBacon commented May 24, 2022

Copy link
Copy Markdown
Contributor

Summary

First part of my require.context feature request. Learn more.

The key here is to add support for importing the contents of a folder recursively and providing to the user as some sort of map, the regular expression is an added layer for refining the results. This means in areas where we'd register a filename, we'd use a directory name instead.

This PR implements the ability to capture require.context invocations in collectDependencies. One could think of this like "language support" for the require.context feature, in the sense that Metro can now make sense of the new syntax.

Because multiple dependencies can use the same module identifier (require.context('[module-id]')) but also provide extra properties like recursive, or a RegExp matcher, we need to change the registry key format to include extra properties. The aforementioned mentioned functionality is primarily implemented in getKeyForDependency.

The entire feature is implemented behind a feature flag unstable_allowRequireContext, e.g.

metro.config.js

const metroConfig = {
  // ...
  transformer: { unstable_allowRequireContext: true }
};

This PR does not implement the entire require.context feature, we'll need a follow up PR implementing changes to the DependencyGraph, DeltaBundler, and metro-runtime.

Test plan

  • Added unit tests.

@facebook-github-bot facebook-github-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 May 24, 2022

@motiz88 motiz88 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good start! I have some comments, hope I didn't miss anything major in this first pass.

We also need registerDependency to distinguish between require.context and other types of dependencies, and between require.context calls with different parameters.

Each of these needs to be registered as a separate dependency (even if they all occur in the same file):

require('./foo');
require.context('./foo');
require.context('./foo', true, /something/);
require.context('./foo', false, /something/);
require.context('./foo', false, /something/, 'lazy');

Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js
Comment thread packages/metro/src/ModuleGraph/worker/__tests__/collectDependencies-test.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/__tests__/collectDependencies-test.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
@EvanBacon EvanBacon requested a review from motiz88 May 25, 2022 17:29

@motiz88 motiz88 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments; we also still need make some changes to DefaultModuleDependencyRegistry.registerDependency + add tests for those.

Comment thread packages/metro-transform-worker/src/index.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/__tests__/collectDependencies-test.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
@EvanBacon EvanBacon mentioned this pull request May 28, 2022
1 task
@EvanBacon EvanBacon marked this pull request as ready for review May 28, 2022 21:50
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label May 28, 2022

@motiz88 motiz88 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix the Flow errors and failing tests, plus update the PR summary? That plus the last comment on the tests, and I think we're ready to land this.

Comment thread packages/metro/src/ModuleGraph/worker/__tests__/collectDependencies-test.js Outdated
EvanBacon added a commit to EvanBacon/metro that referenced this pull request May 29, 2022
@EvanBacon EvanBacon force-pushed the @evanbacon/require-context/collect branch from 25778ac to 7c19f6e Compare June 14, 2022 08:55

@motiz88 motiz88 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really close - some nits, and I would love some more test coverage to ensure all the various permutations of getKeyForDependency are handled correctly, but seems basically good to go from an implementation standpoint.

Comment thread packages/metro/src/DeltaBundler/types.flow.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/__tests__/collectDependencies-test.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/__tests__/collectDependencies-test.js Outdated
Comment thread packages/metro/src/ModuleGraph/worker/collectDependencies.js Outdated
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@motiz88 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@EvanBacon EvanBacon deleted the @evanbacon/require-context/collect branch June 16, 2022 10:11
motiz88 added a commit to motiz88/metro that referenced this pull request Jun 17, 2022
Summary:
Changes `collectDependencies` so it is responsible for providing a key with each extracted dependency. This key should be *stable* across builds and when dependencies are reordered, and *must* be locally unique within a given module.

NOTE: This is a similar concept to the [`key`](https://reactjs.org/docs/lists-and-keys.html#keys) prop in React. It's also used for a similar purpose - `traverseDependencies` is not unlike a tree diffing algorithm.

Previously, the `name` property ( = the first argument to `require` etc) *implicitly* served as this key in both `collectDependencies` and DeltaBundler, but since the addition of `require.context` dependency descriptors in react#821, this is no longer strictly correct. (This diff therefore unblocks implementing `require.context` in DeltaBundler.)

Instead of teaching DeltaBundler to internally re-derive suitable keys from the dependency descriptors - essentially duplicating the logic in `collectDependencies` - the approach taken here is to require `collectDependencies` to return an *explicit* key as part of each descriptor.

NOTE: Keys should be considered completely opaque. As an implementation detail (that may change without notice), we Base64-encode the keys to obfuscate them and deter downstream code from depending on the information in them. (We do this on the assumption that Base64 encoding performs better than hashing.)

Note that it's safe for multiple descriptors to resolve to a single module (as of D37194640 (react@fc29a11)), so from DeltaBundler's perspective it's now perfectly valid for `collectDependencies` to not collapse dependencies at all (e.g. even generate one descriptor per `require` call site) as long as it provides a unique key with each one.

WARNING: This diff exposes a **preexisting** bug affecting optional dependencies (introduced in react#511) - see FIXME comment in `graphOperations.js` for the details. This will require more followup in a separate diff.

Differential Revision: D37205825

fbshipit-source-id: 3559ee6a2f3c30017812ff009f0fe4c442e30029
facebook-github-bot pushed a commit that referenced this pull request Jul 18, 2022
Summary:
Pull Request resolved: #835

Changes `collectDependencies` so it is responsible for providing a key with each extracted dependency. This key should be *stable* across builds and when dependencies are reordered, and *must* be locally unique within a given module.

NOTE: This is a similar concept to the [`key`](https://reactjs.org/docs/lists-and-keys.html#keys) prop in React. It's also used for a similar purpose - `traverseDependencies` is not unlike a tree diffing algorithm.

Previously, the `name` property ( = the first argument to `require` etc) *implicitly* served as this key in both `collectDependencies` and DeltaBundler, but since the addition of `require.context` dependency descriptors in #821, this is no longer strictly correct. (This diff therefore unblocks implementing `require.context` in DeltaBundler.)

Instead of teaching DeltaBundler to internally re-derive suitable keys from the dependency descriptors - essentially duplicating the logic in `collectDependencies` - the approach taken here is to require `collectDependencies` to return an *explicit* key as part of each descriptor.

NOTE: Keys should be considered completely opaque. As an implementation detail (that may change without notice), we hash the keys to obfuscate them and deter downstream code from depending on the information in them.

Note that it's safe for multiple descriptors to resolve to a single module (as of D37194640 (fc29a11)), so from DeltaBundler's perspective it's now perfectly valid for `collectDependencies` to not collapse dependencies at all (e.g. even generate one descriptor per `require` call site) as long as it provides a unique key with each one.

WARNING: This diff exposes a **preexisting** bug affecting optional dependencies (introduced in #511) - see FIXME comment in `graphOperations.js` for the details. This will require more followup in a separate diff.

Changelog: [Internal]

Reviewed By: jacdebug

Differential Revision: D37205825

fbshipit-source-id: 92dc306803e647b25bd576dae02960215fc01da6
facebook-github-bot pushed a commit that referenced this pull request Aug 17, 2022
Summary:
Continued work for adding `require.context` to Metro, which started in #821. The overall design is discussed in microsoft/rnx-kit#1515. The user-facing API is closely modelled on [Webpack's `require.context`](https://webpack.js.org/guides/dependency-management/#requirecontext).

**This feature is experimental and unsupported.** We will document and announce this for general consumption once it's stable.

## How it works

When we encounter a dependency that has `contextParams` (see #821), we will now generate a corresponding *virtual module* in the dependency graph, with dependencies on every file in the file map that matches the context params.

```
┌─────────┐  require.context('./ctx', ...)   ┌−−−−−−−−−−−−−−−−−−┐     ┌──────────┐
│ /bundle │ ───────────────────────────────▶ ╎ <virtual module> ╎ ──▶ │ /ctx/bar │
└─────────┘                                  └−−−−−−−−−−−−−−−−−−┘     └──────────┘
                                               │
                                               │
                                               ▼
                                             ┌──────────────────┐
                                             │     /ctx/foo     │
                                             └──────────────────┘
```

Crucially, we keep this context module up-to-date as file change events come in, so that HMR / Fast Refresh continues to work reliably and transparently. This accounts for the bulk of the complexity of the implementation and tests.

## Main pieces of the implementation

* [collectDependencies] Done in #821: Extract `require.context` calls as dependencies with added metadata. (Behind a flag)
* [metro-file-map] HasteFS: API for querying the file map for the set of files that match a particular resolved context.
* [DeltaBundler] DeltaCalculator: Logic to mark a previously-generated context module as dirty when the set of files it matches has changed.
* [DeltaBundler] graphOperations: Logic to "resolve" context params to a virtual module stored in the graph object, and forward the resolved params to the transformer to generate the module's body and dependencies.
* [DeltaBundler] graphOperations: API for querying the graph for the set of currently active contexts that match a particular file. (Used by DeltaCalculator)
* [metro] transformHelpers, contextModuleTemplates: Logic to generate the contents of a virtual context module by querying the file map. Includes various templates to implement the different `require.context` modes.
* [metro-runtime] `require()`: A stub for `require.context` that helps us throw a meaningful error if the feature is not enabled.

Tests:
* `require-context-test`: a new integration test suite that builds and executes various bundles that use `require.context`. In particular this covers the subset of the `require.context` runtime API that we support.
* `DeltaCalculator-context-test`: a new test suite covering the changes to DeltaCalculator that are specific to `require.context` support.
* `traverseDependencies-test`: expanded and refactored from its previous state. Covers the changes to graphOperations.

## Future work

At the moment, every time we invalidate a context module, we scan the entire file map to find the up-to-date matches and then regenerate the module from scratch (including passing it through the transformer).

Two open areas of investigation are:
1. Optimising the *initial* scan over the file map - e.g. representing it as a path tree to drastically limit the number of files we need to match against.
2. Optimising the incremental case - e.g. directly using the file addition/deletion events we receive from the file map to update the generated module in-place.

At least (2) is essential before we treat this feature as stable.

There's also room to generalise the "virtual modules" concept/infrastructure here to support other use cases. For now everything is very much coupled to the `require.context` use case.

EvanBacon's original PR summary follows.

----

<details>

- Continued work for adding `require.context` to Metro #821
- This PR adds the ability to match files given a "require context". This is similar to the existing method for matching against a regex, but this enables users to search upwards in a directory, search shallow, and match a regex filter.
- Adds ability to pass a `Buffer` to `transformFile` as a sort of virtual file mechanism. This accounts for most the changes in `packages/metro/src/DeltaBundler/Transformer.js`, `packages/metro/src/DeltaBundler/Worker.flow.js`, `packages/metro/src/DeltaBundler/WorkerFarm.js`.
- Since we collapse `require.context` to `require` I've also added a convenience function in dev mode which warns users if they attempt to access `require.context` without enabling the feature flag.
- Made `DeltaCalculator` aware of files being added.
- `graphOperations` has two notable changes:
  1. When resolving dependencies with context, we attach a query to the absolute path (which is used for indexing), this query has a hex hash of the context -- used hex instead of b64 for filename safety.
  2. We pass the require context to `processModule` and inside we transform the dependency different based on the existence of a context module.
- When collecting the delta in `_getChangedDependencies` we now iterate over added and deleted files to see if they match any context modules. This is only enabled when the feature flag is on.
- In `packages/metro/src/lib/contextModule.js` we handle a number of common tasks related to context handling.
- In `packages/metro/src/lib/contextModuleTemplates.js` we generate the virtual modules based on the context. There are a number of different modules that can be created based on the `ContextMode`. I attempted to keep the functionality here similar to Webpack so users could interop bundlers. The most notable missing feature is `context.resolve` which returns the string path to a module, this doesn't appear to be supported in Metro. This mechansim is used for ensuring a module must be explicitly loaded by the user. Instead I wrapped the require values in getters to achieve the same effect.
- We implement the `lazy` mode as well but this requires the user to have async bundles already setup, otherwise the module will throw a runtime error.

### Notice

I've withheld certain optimizations in order to keep this PR simple but still functional. We will want to follow up with a delta file check to require context so we aren't iterating over every file on every update. This feature can be seen in my [test branch](main...EvanBacon:@evanbacon/context/graph-resolver).

In my [test branch](https://github.com/facebook/metro/compare/main...EvanBacon:%40evanbacon/context/graph-resolver?expand=1), I built the feature on top of #835 so I know it works, should only require minor changes to graphOperations to get them in sync.

Pull Request resolved: #822

Test Plan:
I've (motiz88) reviewed and expanded the tests from the original PR, as well as fixed several bugs and removed some accidental complexity that got introduced.

EvanBacon's original PR test plan follows.

 ---

- [ ] Unit tests -- pending motiz88 approving the implementation.

### Local testing

`metro.config.js`
```js
const { getDefaultConfig } = require("expo/metro-config");

const config = getDefaultConfig(__dirname);
const path = require("path");

config.watchFolders = [
  path.resolve(__dirname),
  path.resolve(__dirname, "../path/to/metro"),
];

config.transformer = {
  // `require.context` support
  unstable_allowRequireContext: true,
};

module.exports = config;
```

index.js
```tsx
console.log(require.context("./").keys());
```

Start a dev server, when changes to the file system occur, they should be reflected as automatic updates to the context. This does lead to the issue of having multiple contexts triggering multiple sequential reloads, I don't think this is a blocking issue though.

Also tested with async loading enabled, and the context:  `require.context("./", undefined, undefined, 'lazy')`.

### Behavior

Notable features brought over to ensure this `require.context` functions as close to the original implementation as possible:

- `require.context` does not respect platform extensions, it will return every file matched inside of its context.
- `require.context` can match itself.
- A custom 'empty' module will be generated when no files are matched, this improves the user experience.
- All methods in `require.context` are named to improve the debugging.
- We always match against a `./` prefix. This prefix is also returned in the keys.
- Modules must be loaded by invoking the context as a function, e.g. `context('./module.js')`

</details>

Reviewed By: robhogan

Differential Revision: D38575227

Pulled By: motiz88

fbshipit-source-id: 503f69622f5bebce5d2db03e8f4c4705de169383
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants