metro
metro copied to clipboard
feat: symlinks in node_modules
- resolve symlinks inside "node_modules" directories
- add
follow
function toResolutionContext
type - move "node_modules" logic into the
yieldPotentialPaths
function - feat: scope-only keys in
extraNodeModules
map - fix: always preserve scope when checking
extraNodeModules
map - fix: throw an error if
resolveRequest
fails to resolve a direct import - [BREAKING] refactor the
FailedToResolveNameError
class
Summary
This commit adds support for symlinks in any node_modules
directory. This includes scoped packages. Also, PNPM users rejoice!
The yieldPotentialPaths
function avoids extra work by lazily generating the potential module paths for indirect imports. Also, the resolver now skips over module paths that contain /node_modules/node_modules/
.
The extraNodeModules
map now has support for keys like @babel
to act as a fallback for all modules starting with @babel/
.
Previously, if the resolveRequest
function failed to resolve a direct import, we would continue onto the "node_modules" search logic. The correct behavior is to throw an error.
Partially fixes: #1
Depends on: https://github.com/facebook/jest/pull/7549
Test plan
None yet. Not familiar with Metro's test suite, and I assume you'll want some changes, or deny this altogether in favor of a more informed direction.
I've tested this manually with arbitrary symlinks and PNPM installations.
Thanks for the PR! ❤️
I'm not super familiar with the module resolution logic, so I will need some time to be able to review it properly.
@jeanlauliac: you may have a better knowledge of it, can you take a look?
@rafeca just following up on this as this is blocking our adoption of ReactNative 0.57 (which has better xcode 10 support). This seems to also address https://github.com/facebook/metro/issues/286
any update on progress?
Waiting on the Jest team, but wouldn't mind getting this reviewed soon. 😎
Nice to see some of the this work finally coming together. Really hoping this can land incrementally while adding more features later on. One of the bigger features that is missing right now (correct me if wrong) is the ability to recursively follow symlinks.
Did some initial work on this almost two years ago. See "Recursive detection of symlinked modules" of https://github.com/facebook/react-native/pull/12400 description. Just an attempt to get symlinks in react-native
. Unsure how metro
and node-haste
code changed since then, but perhaps there is something useful still in there.
I don't know if I'd characterize this work as "coming together". It's been almost 7 weeks since this PR was initially pushed up and 2 weeks since I checked in to see the status of this PR. (to which there was no response)
@jeanlauliac @rafeca how can we help get traction on this? It's preventing us from adopting ReactNative 0.57 -- until then we are stuck on 0.56.1
We are also stuck on 0.56, can we do something to help? Same for this issue : https://github.com/facebook/jest/pull/6993
Will look this week. Haven't worked on that code for a while but that should be workable!
@jeanlauliac have you had a chance to take a look? This problem makes a lot of different build environments impossible.
Yes, I've been looking today, thank you for the reminder. Unfortunately I'm working on other projects these days so I struggle to take the time to work on Metro.
Anyway, I think that looks good overall! Even though there are more changes than seem strictly necessary for the feature itself. I was trying to understand where's the "follow" function in HasteFS, and I realised https://github.com/facebook/jest/pull/6993 isn't in yet. So I guess we have to figure it out first.
Also, I notice the follow
function is called only on the folder name, ex. node_modules/foo
rather than the whole, ex. node_modules/foo/some_subfolder
(ie. if a module foo/some_subfolder
was required). Afaik. if follow
is just like realpath
, we should call it on the entire string so that several levels of symlinks would be potentially resolved? In fact, maybe we should always follow
any path that is provided to resolveFile
, since any file might be a symlink to another one after all. It seems to me that approach would cover the node_modules
use case while covering any other symlink case. We should be able to add an integration test for this, that uses a memory-fs
to setup any scenario we need. @rafeca do you know where the resolution logic is being tested these days?
if
follow
is just like realpath, we should call it on the entire string so that several levels of symlinks would be potentially resolved?
In https://github.com/facebook/jest/pull/6993, the Jest team has mentioned they want to make symlinks transparent eventually. This PR is only focused on symlinks in node_modules
. With that in mind, we need to wait on your idea until the Jest team makes a move (or you could coordinate with them).
The main issue with transparent symlinks is the negative effect on performance for developers not utilizing symlinks. For this reason, transparent symlinks should be opt-in. On the other hand, symlinks in node_modules
are only resolved when they exist, so developers not utilizing symlinks won't notice any performance loss.
We should fast-track this PR (and the Jest PR), since many developers have had issues with symlinks in node_modules
, and it's the most common use case for symlinks, IMO.
And if you find time, please add test stubs to this PR (to fill out once the Jest team merges https://github.com/facebook/jest/pull/6993).
Any update here? This has been blocking for us for 3 months now...
I think that's blocked by https://github.com/facebook/jest/pull/6993 as we need to get that one right first, I'll comment there.
Now that https://github.com/facebook/jest/pull/6993 is supplanted by https://github.com/facebook/jest/pull/7549, all symlinks are cached by jest-haste-map
, so it should be easy to add support for "symlinks to modules" in addition to the support for "symlinks in node_modules" added by this PR.
As far as emulating realpath
behavior, we'll need to test the perf effects to decide if it should be enabled by default or not. The follow
function has to find the "link root" of any file path passed to it, which could be expensive when performed thousands of times during module resolution. We could flatten Jest's symlink map into a map of absolute symlink paths to target paths, but it's probably a pain to keep our "flat map" in sync as changes are made.
Any news on the progression of this issue?
Is this issue resolved? Can I expect that now metro builder will treat the symlinked folders as dependencies and include them in haste map? I am currently using the latest RN version 59.5 with witch I can still reproduce the issue. If it is separately handeled in a different branch or module, please guide how I can integrate with my current project setup.
Can we expect this in RN 0.60 / 0.61?
@fr3fou Unlikely. The people over at Jest want "transparent" symlink support, and this PR relies on an implementation of opaque symlinks. 😖
I also haven't been able to get a monorepo setup for React & React Native (expo), I hope this code can be merged soon. I tried; WML, lerna, yarn workspaces and many combinations of these with no luck.
Hey all! Joining this convo for the first time.
What's the status here? I'm super interested in using this functionality!
@aleclarson how do they want symlink transparent? And how can we do this? I would love to help.
Hey @aleclarson, would you consider opening a separate PR to just get this particular fix merged, or is it inseparable to other changes.
fix: always preserve scope when checking extraNodeModules map
Rebased onto v0.56
Just another dev and metro user who was hoping to use Lerna hopping on here to say that this is a blow to my plans for wanting a component library monorepo for RN, that it isn't merged in yet. Would love to see this feature looked at by the metro team and liaise with jest to get jest/9351 in too. Until then, sadness :'(
@James-Reed-cognito In the meantime, you can try haul
@jsamr Ooh interesting, though looks like it doesnt have hot reload/fast refresh?
@jsamr I actually found a way to make this work by tweaking the metro config. Should I detail this here or perhaps add a PR to add a section in the docs for it, do you reckon?
@James-Reed-cognito definitely post here. This issue is the top search result for others experiencing the issue, if you have a good solution that's easy and straightforward you'll probably help lots of people.
Alright then, changing my metro.config.js to this seemed to make everything work:
const path = require('path');
module.exports = {
resolver: {
extraNodeModules: new Proxy(
{},
{
get: (target, name) => path.join(process.cwd(), `node_modules/${name}`),
},
),
},
projectRoot: path.resolve(__dirname),
watchFolders: [path.resolve(__dirname, '../<component_package>')],
transformer: {
getTransformOptions: async () => ({
transform: {
experimentalImportSupport: false,
inlineRequires: false,
},
}),
},
};
All the credit actually goes to a friend of mine who gave me this solution. the watchFolders
is the important part - just add all the packages that are dependencies into that.
Hi everyone. Any updates on this? Have been trying to make a working monorepo with yarn workspaces for several days, no success...
@vlad-dewitt I recently set up a RN monorepo and found this blog post very useful https://medium.com/@ratebseirawan/react-native-0-63-monorepo-walkthrough-36ea27d95e26