feat: symlinks in node_modules

[aleclarson]

• resolve symlinks inside "node_modules" directories
• add follow function to ResolutionContext 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.

[rafeca]

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?

[nickarora]

@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?

[aleclarson]

Waiting on the Jest team, but wouldn't mind getting this reviewed soon. 😎

[Swaagie]

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.

[nickarora]

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

[RomualdPercereau]

We are also stuck on 0.56, can we do something to help? Same for this issue : https://github.com/facebook/jest/pull/6993

[jeanlauliac]

Will look this week. Haven't worked on that code for a while but that should be workable!

[Nantris]

@jeanlauliac have you had a chance to take a look? This problem makes a lot of different build environments impossible.

[jeanlauliac]

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?

[aleclarson]

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).

[nickarora]

Any update here? This has been blocking for us for 3 months now...

[jeanlauliac]

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.

[aleclarson]

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.

[RomualdPercereau]

Any news on the progression of this issue?

[SachinB-Droisys]

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.

[fr3fou]

Can we expect this in RN 0.60 / 0.61?

[aleclarson]

@fr3fou Unlikely. The people over at Jest want "transparent" symlink support, and this PR relies on an implementation of opaque symlinks. 😖

[rootedsoftware]

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.

[swain]

Hey all! Joining this convo for the first time.

What's the status here? I'm super interested in using this functionality!

[phsantiago]

@aleclarson how do they want symlink transparent? And how can we do this? I would love to help.

[SpotHimesh]

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

[aleclarson]

Rebased onto v0.56

[James-Reed-cognito]

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 :'(

[jsamr]

@James-Reed-cognito In the meantime, you can try haul

[James-Reed-cognito]

@jsamr Ooh interesting, though looks like it doesnt have hot reload/fast refresh?

[James-Reed-cognito]

@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?

[jrylan]

@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.

[James-Reed-cognito]

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.

[vizet]

Hi everyone. Any updates on this? Have been trying to make a working monorepo with yarn workspaces for several days, no success...

[axelander]

@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