feat: add require.context support#822
Conversation
motiz88
left a comment
There was a problem hiding this comment.
I would fold this into the upcoming PR which uses this to implement require.context, to make sure any edge cases are ironed out & tested - I don't want to have to ship breaking changes to DependencyGraph at that point or integration bugs to slip through. Overall looks great though.
Co-authored-by: Moti Zilberman <motiz88@gmail.com>
53fa3f3 to
360ee19
Compare
resolveContext method for matching filesrequire.context support
motiz88
left a comment
There was a problem hiding this comment.
@EvanBacon this seems broadly in line with what we've discussed. Here's a round of comments. I would love to take another look once there are tests for the new functionality and CI is passing.
| // NOTE(EvanBacon): It's unclear if we should use `import` or `require` here so sticking | ||
| // with the more stable option (`require`) for now. |
There was a problem hiding this comment.
My guess is require, but the deciding factor is what Webpack does.
revert broken feedback Updated comment Added tests for HasteFS Update HasteFS-test.js Add contextModuleTemplates tests fixup types Update index.js Drop getTransformFn drop unused Update types.flow.js Added more tests inputPath -> from Test require.context fixup
16796e3 to
7b78806
Compare
motiz88
left a comment
There was a problem hiding this comment.
Incomplete pass of feedback, will make more time next week.
| // `require.context` | ||
| const {contextParams} = dep.data; | ||
| if (contextParams) { | ||
| contextParams.from = path.join(parentPath, '..', dep.name); |
There was a problem hiding this comment.
Mutating contextParams here is a code smell - let's treat the input to this function as read-only.
Also, is dep.name guaranteed to be a relative path? Can you add a comment to this effect?
There was a problem hiding this comment.
In the default case we resolve dep.name as a path component, this assumption builds on how the existing functionality works.
|
@motiz88 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
robhogan
left a comment
There was a problem hiding this comment.
(Imported PRs need internal review by someone other than the importer, sharing feedback here for transparency)
Left a few nits and minor comments around path handling that need explanation if not changes, but substantively this looks good to me. Thanks for working on this - must be one of the biggest feature contributions we've ever had!
| const filePath = fastPath.relative(root, file); | ||
|
|
||
| const isRelative = | ||
| filePath && !filePath.startsWith('..') && !path.isAbsolute(filePath); |
There was a problem hiding this comment.
Comment:
filePath.startsWith('..') assumes normalisation, which seems to be safe in practice but I can't see it explicitly mentioned in the Node JS docs for relative (as it is for join, for example).
I wondered about the necessity of the isAbsolute check too, given we've obtained this path using relative(), but I guess that's for Windows, eg for two files on different drives?
There was a problem hiding this comment.
isAbsolute check appears to be extraneous now.
There was a problem hiding this comment.
I'm not sure normalization is required for the the .. check to work as this paradigm is cross-platform.
There was a problem hiding this comment.
I'm not sure normalization is required for the the
..check to work as this paradigm is cross-platform.
If it's not normalised it might not appear at the start of the path - eg foo/../../bar is valid but normalises to ../bar
There was a problem hiding this comment.
isAbsolute check appears to be extraneous now.
I think it's still necessary for Windows tbh, looks like we get absolute paths back when there's no valid relative path between locations:
node -e "console.log(path.win32.relative('C:/foo', 'D:/bar'))";
D:\bar
|
@motiz88 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
I've noticed this PR says
Are there plans to stabilise this? I've opened react/react-native#41421 in case it's already there. |
It's been 1½ years since the merge of react/metro#822 and my hope is that this feature is ready to be documented and made available through types.
|
The main thing holding us back was that the pre-symlink filesystem implementation didn't support enumerating a subtree, so |
|
I've been experimenting with this feature recently, and it's fantastic! However, I believe I've come across a potential issue. To be honest, I'm not entirely sure if it's an actual problem or if I'm simply using it incorrectly. My goal is to dynamically import images. In my application, I'm displaying a list of cards, and within each card, there's an area where I'm supposed to render a pre-defined image. I have a collection of images stored locally, and for each card, I intend to render one of these images. The Do you think I might be misunderstanding something? |
|
For anyone looking for a copy+paste |
|
This feature is great thanks. But a shame it doesn't work with anything other than relative paths (and maybe absolute looking at the tests but that doesn't work for a monorepo). Has to be: const req = require.context(
'../../../../../node_modules/@metamask/contract-metadata/images',
false,
/\.svg$/,
)Which is not only dependent on the location of the file, but also where a package manager decides to install something in a monorepo (in the root or project). It would be ideal if we could do: const req = require.context(
'@metamask/contract-metadata/images',
false,
/\.svg$/,
)It also doesn't seem to work with Off topic: Also I read that RN has dynamic |
Replace @types/webpack-env dependency with custom Metro type definitions, following the same approach used by Expo. Metro's require.context API intentionally matches webpack's API by design, but using webpack types caused concerns about API differences. This change: - Adds metro-env.d.ts with Metro-specific types in __MetroModuleApi namespace - Exports types via @storybook/react-native/metro-env - Auto-injects reference directive in generated storybook.requires.ts files - Removes @types/webpack-env dependency References: - Expo's approach: https://github.com/expo/expo/blob/main/packages/expo/types/metro-require.d.ts - Metro require.context PR: react/metro#822 - Metro HMR Flow types: https://github.com/facebook/metro/blob/main/packages/metro-runtime/src/polyfills/require.js.flow
Replace @types/webpack-env dependency with custom Metro type definitions, following the same approach used by Expo. Metro's require.context API intentionally matches webpack's API by design, but using webpack types caused concerns about API differences. This change: - Adds metro-env.d.ts with Metro-specific types in __MetroModuleApi namespace - Exports types via @storybook/react-native/metro-env - Auto-injects reference directive in generated storybook.requires.ts files - Removes both @ts-ignore comments from generated code References: - Expo's approach: https://github.com/expo/expo/blob/main/packages/expo/types/metro-require.d.ts - Metro require.context PR: react/metro#822 - Metro HMR Flow types: https://github.com/facebook/metro/blob/main/packages/metro-runtime/src/polyfills/require.js.flow
Replace @types/webpack-env dependency with custom Metro type definitions, following the same approach used by Expo. Metro's require.context API intentionally matches webpack's API by design, but using webpack types caused concerns about API differences. This change: - Adds metro-env.d.ts with Metro-specific types in __MetroModuleApi namespace - Exports types via @storybook/react-native/metro-env - Auto-injects reference directive in generated storybook.requires.ts files - Removes both @ts-ignore comments from generated code References: - Expo's approach: https://github.com/expo/expo/blob/main/packages/expo/types/metro-require.d.ts - Metro require.context PR: react/metro#822 - Metro HMR Flow types: https://github.com/facebook/metro/blob/main/packages/metro-runtime/src/polyfills/require.js.flow
Replace @types/webpack-env dependency with custom Metro type definitions, following the same approach used by Expo. Metro's require.context API intentionally matches webpack's API by design, but using webpack types caused concerns about API differences. This change: - Adds metro-env.d.ts with Metro-specific types in __MetroModuleApi namespace - Exports types via @storybook/react-native/metro-env - Auto-injects reference directive in generated storybook.requires.ts files - Removes both @ts-ignore comments from generated code References: - Expo's approach: https://github.com/expo/expo/blob/main/packages/expo/types/metro-require.d.ts - Metro require.context PR: react/metro#822 - Metro HMR Flow types: https://github.com/facebook/metro/blob/main/packages/metro-runtime/src/polyfills/require.js.flow
* fix: replace @ts-ignore with Metro-specific type definitions Replace @types/webpack-env dependency with custom Metro type definitions, following the same approach used by Expo. Metro's require.context API intentionally matches webpack's API by design, but using webpack types caused concerns about API differences. This change: - Adds metro-env.d.ts with Metro-specific types in __MetroModuleApi namespace - Exports types via @storybook/react-native/metro-env - Auto-injects reference directive in generated storybook.requires.ts files - Removes both @ts-ignore comments from generated code References: - Expo's approach: https://github.com/expo/expo/blob/main/packages/expo/types/metro-require.d.ts - Metro require.context PR: react/metro#822 - Metro HMR Flow types: https://github.com/facebook/metro/blob/main/packages/metro-runtime/src/polyfills/require.js.flow * fix: module.hot --------- Co-authored-by: Daniel Williams <dannyhyunsoowilliams@gmail.com>
Continued work for adding
require.contextto Metro, which started in #821. The overall design is discussed in microsoft/rnx-kit#1515. The user-facing API is closely modelled on Webpack'srequire.context.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.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
require.contextcalls as dependencies with added metadata. (Behind a flag)require.contextmodes.require(): A stub forrequire.contextthat 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 userequire.context. In particular this covers the subset of therequire.contextruntime API that we support.DeltaCalculator-context-test: a new test suite covering the changes to DeltaCalculator that are specific torequire.contextsupport.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:
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.contextuse case.@EvanBacon's original PR summary follows.
Details
Summary
require.contextto Metro feat: add support for capturing require.context in dependency collector #821BuffertotransformFileas a sort of virtual file mechanism. This accounts for most the changes inpackages/metro/src/DeltaBundler/Transformer.js,packages/metro/src/DeltaBundler/Worker.flow.js,packages/metro/src/DeltaBundler/WorkerFarm.js.require.contexttorequireI've also added a convenience function in dev mode which warns users if they attempt to accessrequire.contextwithout enabling the feature flag.DeltaCalculatoraware of files being added.graphOperationshas two notable changes:processModuleand inside we transform the dependency different based on the existence of a context module._getChangedDependencieswe 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.packages/metro/src/lib/contextModule.jswe handle a number of common tasks related to context handling.packages/metro/src/lib/contextModuleTemplates.jswe generate the virtual modules based on the context. There are a number of different modules that can be created based on theContextMode. I attempted to keep the functionality here similar to Webpack so users could interop bundlers. The most notable missing feature iscontext.resolvewhich 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.lazymode 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.
In my test branch, 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.
Test plan
Local testing
metro.config.jsindex.js
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.contextfunctions as close to the original implementation as possible:require.contextdoes not respect platform extensions, it will return every file matched inside of its context.require.contextcan match itself.require.contextare named to improve the debugging../prefix. This prefix is also returned in the keys.context('./module.js')