Skip to content
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: symlinks in node_modules #257

Closed
wants to merge 2 commits into from
Closed

Conversation

aleclarson
Copy link
Contributor

@aleclarson aleclarson commented Sep 18, 2018

  • 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: jestjs/jest#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.

@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 Sep 18, 2018
@rafeca
Copy link
Contributor

rafeca commented Sep 24, 2018

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?

eml added a commit to feedfm/react-native-feed-media-audio-player that referenced this pull request Oct 4, 2018
The react native bundler, 'metro', doesn't follow symlinks. This meant
that in the example app we couldn't do

import Foo from "react-native-feed-media-audio-player"

when the library was installed via 'npm install ../package', because
that npm a symlink in
'node_modules/react-native-feed-media-audio-player' and metro wouldn't
follow it. Maddening. Hopefully this will be fixed with:

facebook/metro#257

This patch inverts things, so the top level 'package' directory is
a symlink, and the node_modules/react... is the actual source. Now
we can run the example app while changing code in the library.
@nickarora
Copy link

@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 #286

any update on progress?

@aleclarson
Copy link
Contributor Author

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

@Swaagie
Copy link

Swaagie commented Nov 1, 2018

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 facebook/react-native#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
Copy link

nickarora commented Nov 1, 2018

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
Copy link

We are also stuck on 0.56, can we do something to help? Same for this issue : jestjs/jest#6993

@jeanlauliac
Copy link

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

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@Nantris
Copy link

Nantris commented Nov 27, 2018

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

@jeanlauliac
Copy link

jeanlauliac commented Nov 28, 2018

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 jestjs/jest#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
Copy link
Contributor Author

aleclarson commented Nov 28, 2018

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 jestjs/jest#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 jestjs/jest#6993).

@nickarora
Copy link

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

@jeanlauliac
Copy link

jeanlauliac commented Dec 17, 2018

I think that's blocked by jestjs/jest#6993 as we need to get that one right first, I'll comment there.

@aleclarson
Copy link
Contributor Author

aleclarson commented Dec 26, 2018

Now that jestjs/jest#6993 is supplanted by jestjs/jest#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.

if (result.type === 'resolved') {
return result.resolution;
/** Generate the potential module paths */
function* genModulePaths(
Copy link
Contributor

Choose a reason for hiding this comment

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

fancy

(scopeName ? extraNodeModules[scopeName] : void 0);

if (parent) {
yield path.join(follow(path.join(parent, packageName)), ...bits);
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this need to be path.join(parent, 'node_modules', packageName)? If not, why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous code dealing with extraNodeModules didn't do that, so I didn't either.

The idea is that a package name can be mapped to a node_modules directory it may exist in.

extraNodeModules: {
  react: '/a/b/c/node_modules'
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, nevermind. I think I misread the old code.

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

This is awesome. Here are the next steps:

  • Get the Jest PR merged.
  • We'll publish a new release of jest-haste-map and update Metro to use it (this may take a while if we also have to update Jest for RN at FB).
  • Merge this PR.
  • Make a Metro release.
  • Update the react-native-cli to use the new version of Metro.

@aleclarson aleclarson force-pushed the symlinks branch 2 times, most recently from 205ed77 to b46da65 Compare June 1, 2020 16:05
- resolve symlinks inside "node_modules" directories
- add `follow` function to `ResolutionContext` type
- move "node_modules" logic into the `genModulePaths` method
- 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

This commit adds support for symlinks in any `node_modules` directory. This includes scoped packages. Also, PNPM users rejoice!

The `genModulePaths` method 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.
We cannot assume that `filePath` is _not_ a directory.
@aleclarson
Copy link
Contributor Author

Rebased onto v0.56

@James-Reed-cognito
Copy link

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
Copy link

jsamr commented Sep 17, 2020

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

@James-Reed-cognito
Copy link

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

@James-Reed-cognito
Copy link

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

@remyrylan
Copy link

@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
Copy link

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
Copy link

vizet commented Dec 7, 2020

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

@axelander
Copy link

@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

@es-lynn
Copy link

es-lynn commented Dec 15, 2020

I was able to get it to work using this guide: https://medium.com/@dushyant_db/how-to-use-lerna-with-react-native-1eaa79b5d8ec

const path = require('path');
const watchFolders = [
  // YOUR relative path to the packages directory
  path.resolve(__dirname + '/../packages'),
];

module.exports = {
  transformer: {
    // ......
  },
  watchFolders,
};

@ajanuar
Copy link

ajanuar commented Mar 13, 2021

Any update for this PR?

@joe06102
Copy link

I'd recommend @callback/repack to anyone who wants symlinks to work with react-native since the metro maintainers do not work actively on this thread.

@zkochan
Copy link

zkochan commented Feb 17, 2022

In case someone is looking for a workaround, pnpm has now a node-linker=hoisted option that creates a regular node_modules, without symlinks.

@evelant
Copy link

evelant commented Sep 7, 2022

Symlinks were the very first issue filed on this repo over 5 and a half years ago. I think it's safe to say they're never going to happen from the metro maintainers. There is an alternative now however! https://microsoft.github.io/rnx-kit/docs/tools/metro-resolver-symlinks works perfectly to make metro follow symlinks. Got my app up and running on a pnpm workspace build by EAS with just a little bit of effort.

Edit: and by "little bit of effort" I mean add public-hoist-pattern[]=*react-native* (and others) to .npmrc for all the packages that still use hardcoded paths.

@robhogan
Copy link
Contributor

robhogan commented Sep 7, 2022

I know it's been a while but believe it or not, we're actually working on this now. That was part of the motivation for decoupling from jest-haste-map.

@evelant
Copy link

evelant commented Sep 8, 2022

I know it's been a while but believe it or not, we're actually working on this now. That was part of the motivation for decoupling from jest-haste-map.

I'm not asking this to insult anybody, I'm genuinely curious -- Why has it taken 5+ years to add support for symlinks despite a community PR and every major JS package manager using them? Is there anything that can be done to help move Metro forward into the modern JS ecosystem?

As a user of metro it's getting more and more difficult to use as it's pretty far behind the capabilities and best practices of all modern tooling. There are pain points with bundle sizes, bundling speed, ESM support, monorepos, etc all of which are long solved problems in modern JS tooling. I think there's a sincere desire from the community to collaborate with the metro team on bringing metro into the modern JS tooling world. How can we make that happen?

@robhogan
Copy link
Contributor

robhogan commented Sep 8, 2022

That's a fair question - I'm not sure this is the best place for a proper answer, but broadly - personnel changes, priority shifts, challenges posed by Metro's deep integration into Meta infrastructure in ways not visible from GitHub, and sometimes - as in this case with Watchman and then Jest - technical dependencies outside this project.

What I can say is that the team behind Metro at Meta is larger, growing and more engaged in Metro than it has been in several years. We know we have a lot of catching up to do, and we can do a better job of communicating what we're doing, but we're investing in and optimistic about Metro's future.

As far as collaboration is concerned - it's very much appreciated, we're open to it - I'd just advise opening an issue to discuss anything non-trivial before spending time on it, because this repo is currently the foundation of a Jenga-like tower of infrastructure at Meta and some changes may be more complicated than they appear - which is actually one of the things we're looking to sort out.

So, apologies, but please bear with us :)

@robhogan
Copy link
Contributor

Thanks for this @aleclarson - I know it’s been several years, but this PR and all of the discussion is where we started when designing the general symlinks approach now in main. I’m closing this because it’s superseded by changes landed, but thanks for the groundwork. 🙏

@robhogan robhogan closed this Feb 22, 2023
@aleclarson
Copy link
Contributor Author

@robhogan Congrats on your efforts :)

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Follow symlinks?