New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add require.context
support
#822
feat: add require.context
support
#822
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the default case we resolve dep.name
as a path component, this assumption builds on how the existing functionality works.
packages/metro/src/DeltaBundler/__tests__/DeltaCalculatorContext-test.js
Outdated
Show resolved
Hide resolved
I've come to believe we should merge the existing DeltaCalculator + traverseDependencies test suites. It can be done fairly mechanically by rewriting the traverseDependencies tests we have in terms of a DeltaCalculator instance, but it's a chance to also rethink the way we mock out the transformer/filesystem and hopefully simplify things some more. Anyway, I won't block this PR on that. |
@motiz88 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@motiz88 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isAbsolute check appears to be extraneous now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
It's been 1½ years since the merge of facebook/metro#822 and my hope is that this feature is ready to be documented and made available through types.
It's been 1½ years since the merge of facebook/metro#822 and my hope is that this feature is ready to be documented and made available through types.
I've noticed this PR says
Are there plans to stabilise this? I've opened facebook/react-native#41421 in case it's already there. |
It's been 1½ years since the merge of facebook/metro#822 and my hope is that this feature is ready to be documented and made available through types.
It's been 1½ years since the merge of facebook/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 |
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'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.context
calls as dependencies with added metadata. (Behind a flag)require.context
modes.require()
: A stub forrequire.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 userequire.context
. In particular this covers the subset of therequire.context
runtime API that we support.DeltaCalculator-context-test
: a new test suite covering the changes to DeltaCalculator that are specific torequire.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:
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.
Summary
require.context
to Metro feat: add support for capturing require.context in dependency collector #821Buffer
totransformFile
as 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.context
torequire
I've also added a convenience function in dev mode which warns users if they attempt to accessrequire.context
without enabling the feature flag.DeltaCalculator
aware of files being added.graphOperations
has two notable changes:processModule
and inside we transform the dependency different based on the existence of a context module._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.packages/metro/src/lib/contextModule.js
we handle a number of common tasks related to context handling.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 theContextMode
. I attempted to keep the functionality here similar to Webpack so users could interop bundlers. The most notable missing feature iscontext.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.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.
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.js
index.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.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.require.context
are named to improve the debugging../
prefix. This prefix is also returned in the keys.context('./module.js')