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

Follow symlinks? #1

Closed
jeanlauliac opened this issue Feb 27, 2017 · 241 comments
Closed

Follow symlinks? #1

jeanlauliac opened this issue Feb 27, 2017 · 241 comments

Comments

@jeanlauliac
Copy link

jeanlauliac commented Feb 27, 2017

Do you want to request a feature or report a bug?

Feature

What is the current behavior?

I'm removing a test from DependencyGraph-test that looked like so, but wasn't actually working after I fixed problems with the fs mocks:

    it('should work with packages with symlinked subdirs', function() {
      var root = '/root';
      setMockFileSystem({
        'symlinkedPackage': {
          'package.json': JSON.stringify({
            name: 'aPackage',
            main: 'main.js',
          }),
          'main.js': 'lol',
          'subdir': {
            'lolynot.js': 'lolynot',
          },
        },
        'root': {
          'index.js': [
            '/**',
            ' * @providesModule index',
            ' */',
            'require("aPackage/subdir/lolynot")',
          ].join('\n'),
          'aPackage': { SYMLINK: '/symlinkedPackage' },
        },
      });

      var dgraph = new DependencyGraph({
        ...defaults,
        roots: [root],
      });
      return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').then(function(deps) {
        expect(deps)
          .toEqual([
            {
              id: 'index',
              path: '/root/index.js',
              dependencies: ['aPackage/subdir/lolynot'],
              isAsset: false,
              isJSON: false,
              isPolyfill: false,
              resolution: undefined,
              resolveDependency: undefined,
            },
            {
              id: 'aPackage/subdir/lolynot.js',
              path: '/root/aPackage/subdir/lolynot.js',
              dependencies: [],
              isAsset: false,
              isJSON: false,
              isPolyfill: false,
              resolution: undefined,
              resolveDependency: undefined,
            },
          ]);
      });
    });

What is the expected behavior?

Reintroduce the test and verify symlinks work.

@lacker
Copy link

lacker commented Feb 27, 2017

symlinks not working in React Native is one of the biggest RN complaints, so if my understanding is correct and this would fix that, this would be great!

@jeanlauliac
Copy link
Author

I think we won't have time to deal with this anytime soon unfortunately. I'd love to reopen once we have a strong case.

@ericwooley
Copy link

Can we leave this open until it's resolved?

This is a huge PITA for code sharing, and developing modules. I'm not sure what your looking for in terms of a strong case, but it's definitely a major pain point for me, and module developers.

Currently I am trying to use lerna, and react-primitives to reorganize a project boilerplate to maximize code reuse, while maintaining upgradability by rn.

Lerna works by symlinking and installing your packages in a mono repo, which is an incredibly helpful pattern for code reuse, and completely broken when the react native packager won't follow symlinks.

@cpojer
Copy link
Contributor

cpojer commented Jun 15, 2017

Yeah, it should be open. We should find a fix but we are not actively invested in making this change ourselves right now. If the community can find a reasonable solution, we should merge it and make it work.

@cpojer cpojer reopened this Jun 15, 2017
@ericwooley
Copy link

ericwooley commented Jun 15, 2017

I suspect it has to do with this line
according to the docs you may also need to check if it's a symlink. EG

//...
 this._moduleResolver = new ModuleResolver({
      dirExists: filePath => {
        try {
           var stats = fs.lstatSync(filePath);
          return  stats.isDirectory() || stats.isSymbolicLink();
        } catch (e) {}
        return false;
      },
// ...

Though I am short on time for digesting how to get started with this project at the moment. I may have more time over the weekend for a proper PR.

Or if someone else gets the chance to play around with it, that would be awesome.

I never have enough time to accomplish all the things I want to do :(

Edit: added "may", as the docs don't explicitly say you isDirectory() won't return true on a symlink, but I believe that is the case.

Edit^2: https://github.com/ericwooley/symlinkTest You do need to explicitly check on OSX Sierra at least, not sure about linux or windows.

Edit^3: the commit where the test was removed c1a8157

@haggholm
Copy link
Contributor

I'd love to reopen once we have a strong case.

With npm 5, all local installs (npm i ../foo) are now symlinked. This makes it pretty rough to work with projects that have local dependencies. (That’s on top of the monorepo scenario.)

@jeanlauliac
Copy link
Author

jeanlauliac commented Jun 16, 2017

Edit^3: the commit where the test was removed c1a8157

The test was removed because it was already broken for a long time, unfortunately (I forgot to put that in the changeset description apparently). Now I realised I shouldn't have removed it completely, only make verify the broken behavior.

I suspect it has to do with this line

dirExists is only used for debugging, not for resolution. The resolution is done based on the HasteFS instance provided by the jest-haste-map module. I believe, but may well be wrong, that a fix for symlinks will have to be implemented in jest-haste-map, both for its watchman and plain crawlers, as well for its watchman and plain watch modes. Since, last time I've heard (might have changed!), watchman doesn't handle symlink, we were hampered in these efforts.

@ericwooley
Copy link

dirExists is only used for debugging, not for resolution. The resolution is done based on the HasteFS instance provided by the jest-haste-map module. I believe, but may well be wrong, that a fix for symlinks will have to be implemented in jest-haste-map, both for its watchman and plain crawlers, as well for its watchman and plain watch modes. Since, last time I've heard (might have changed!), watchman doesn't handle symlink, we were hampered in these efforts.

Interesting.

I am pretty confused by this project, as I am not sure how it is imported by RN. But I manually edited this change into node_modules/react-native/packager/src/node-haste/dependencyGraph.js to use my suggested code and no longer got errors about symlinked modules not being found. I got a ton of other errors, i think related to multiple installs of react-native, but I seemed to have gotten much further in the process.

I also put a console log there to offer some insight, and it was definitely logging. So that line appears to be used by the react-native project while doing real work (as opposed to debugging).

After much frustration with this issue, it ended up being easier to PR the libs i need to work with haul, rather than try to tackle this problem. I wish I had more time for this issue, but it appears to be a pretty rough one to solve, given what @jeanlauliac said.

Hopefully this gets officially resolved soon. as I would much prefer to use the built in solution.

On a side note, haul seems to have fixed all the symlink issues. Is there a downside to haul that you know of? It seems like it could be something that could be officially brought into the fold.

@ghost
Copy link

ghost commented Jun 20, 2017

Something that half-works is not symbolic linking but hard linking. You can't hard link directories, but you can use cp to copy all the directories and hard link every file rather than copy it with -al, e.g. cp -al <sourceDirectory> <destinationDirectory>.

The only catch is that it seems in order to trigger rebundling I have to save the changed file (even though the action of saving isn't changing the contents of the file).

@ghost
Copy link

ghost commented Jun 20, 2017

@ericwooley Can you explain how your PR, haul, and storybooks (?) relate to the react-native symbolic link issue?

@ericwooley
Copy link

@bleighb

Haul is an alternative to the react-native packager that doesn't have issues with symlinks. They reference this issue because those PR's and issues are about issues with symlinking and the metro bundler.

As to your cp -r issue, you might have better luck with rsync. Which is something I considered at one point.

@kschzt
Copy link

kschzt commented Jun 21, 2017

I'm in the lerna boat, too. The way I finally worked around this issue was to just explicitly add the packages in our monorepo to rn-cli.config.js, instead of having them as dependencies in the RN project's package.json (and symlinked into node_modules)

var config = {
  getProjectRoots() {
    return [
      path.resolve(__dirname), 
      path.resolve(__dirname, '../some-other-package-in-lerna-monorepo')
    ]
  }
}

May have changed for Metro. HTH

@ericwooley
Copy link

ericwooley commented Jun 21, 2017

@kschzt I will have to try that solution tonight.

I imagine that there will be more pressure on this issue once yarn workspaces are released. Until then, your solution may be the only way forward.

@haggholm
Copy link
Contributor

haggholm commented Jun 22, 2017

I was unable to get things to work in something like a monorepo when trying @kschzt’s approach. If I have module A as a dependency and point rn-cli.config.js to it explicitly, then it will not be found by packages requiring it from inside the ‘root’ repo. In fact, the packager doesn’t even look! The structure is probably fairly obvious from a log excerpt:

Looking for JS files in
   /mnt/scratch/shared/petter/owl-devenv/client/root/local
   /mnt/scratch/shared/petter/owl-devenv/client/root/local/@client
   /mnt/scratch/shared/petter/owl-devenv/client/root/local/@client/util
   /mnt/scratch/shared/petter/owl-devenv/client/root 

React packager ready.

Loading dependency graph, done.
warning: the transform cache was reset.
error: bundling: UnableToResolveError: Unable to resolve module `@client/util/memoizeOnce` from `/mnt/scratch/shared/petter/owl-devenv/client/root/node_modules/@client/store/bind.js`: Module does not exist in the module map or in these directories:
  /mnt/scratch/shared/petter/owl-devenv/client/root/node_modules/@client/util

Why does it not check to see if the module exists inside the local/ directory? Beats me.

(react-native v0.45.0.)

@ericwooley
Copy link

ericwooley commented Jun 23, 2017

@kschzt @haggholm

How are you importing your files

  1. import whatever from '../../otherPackage/someModules'?
  2. import whatever from 'otherPackage/someModules'?
  3. import whatever from 'someModules'?

I'm not really sure how roots work in this context, but the way your importing may be a factor. Or are you using the @providesModule syntax?

@haggholm
Copy link
Contributor

@ericwooley 2(ish): import whatever from '@foo/bar/baz, in this case, with baz having .js, .android.js, and .ios.js versions for different platforms.

Both the project and I are new to React Native (migrating an existing web app); I’m not familiar with the @providesModule syntax. Perhaps I should look into that and see if it works…

@pocesar
Copy link

pocesar commented Jun 28, 2017

funny, it used to work (import { something } from 'symlinked-package'), now it doesn't anymore after upgrading react native from 0.43 to 0.45, anything changed in the packager from there?

@AshCoolman
Copy link

This lerna project seems to work with Symlinks too (react-native 0.40)

https://github.com/samcorcos/learna-react-native

@ericwooley
Copy link

I have played with that too @AshCoolman, and it does seem to work, but setting it up in a similar way with other versions has not worked.

@AshCoolman
Copy link

@ericwooley My experience too

@ktj
Copy link

ktj commented Jul 31, 2017

Is the only solution at the moment to use haul or some custom shell scripts?

@sannajammeh
Copy link

sannajammeh commented Jul 5, 2023

public-hoist-pattern[]=*metro*

This does not contain monorepo configs, but should work the same. I'll test later today.

Updating for pnpm & Expo SDK 49 users:

npmrc:

public-hoist-pattern[]=*metro*
public-hoist-pattern[]=*@babel*

Metro config:

// Learn more https://docs.expo.io/guides/customizing-metro
const { getDefaultConfig } = require("expo/metro-config");
const MetroSymlinksResolver = require("@rnx-kit/metro-resolver-symlinks");

/** @type {import('expo/metro-config').MetroConfig} */
const config = getDefaultConfig(__dirname, {
  // [Web-only]: Enables CSS support in Metro.
  isCSSEnabled: true,
});

config.resolver.resolveRequest = new MetroSymlinksResolver();

module.exports = config;

@minhchu
Copy link

minhchu commented Jul 6, 2023

Hi @sannajammeh
Thanks for your solution but I did a fresh install of expo and it still didn't work. Here are my steps:

  1. pnpm dlx create-expo-app --template # typescript + navigation
  2. rm -rf node_modules
  3. update .npmrc and metro.config.js with your above suggestion

When I run npm start and open the app in Expo GO, it is still showing something like Cannot read properties of undefined (reading 'addHelper'). Can you upload a working repo with your config ?

@sannajammeh
Copy link

Hi @sannajammeh Thanks for your solution but I did a fresh install of expo and it still didn't work. Here are my steps:

  1. pnpm dlx create-expo-app --template # typescript + navigation
  2. rm -rf node_modules
  3. update .npmrc and metro.config.js with your above suggestion

When I run npm start and open the app in Expo GO, it is still showing something like Cannot read properties of undefined (reading 'addHelper'). Can you upload a working repo with your config ?

This is working on my PC. https://github.com/sannajammeh/expo-49-test/tree/pnpm
Be sure to degit the branch pnpm and not the main.

If its not working, try force installing with pnpm i --force as some deps might still have issues.

@rollingmoai
Copy link

Hi @sannajammeh Thanks for your solution but I did a fresh install of expo and it still didn't work. Here are my steps:

  1. pnpm dlx create-expo-app --template # typescript + navigation
  2. rm -rf node_modules
  3. update .npmrc and metro.config.js with your above suggestion

When I run npm start and open the app in Expo GO, it is still showing something like Cannot read properties of undefined (reading 'addHelper'). Can you upload a working repo with your config ?

Same here, except I'm using the bare typescript template.

@minhchu
Copy link

minhchu commented Jul 28, 2023

@rollingmoai hi, I tried sannajammeh's solution and it works:

This is working on my PC. https://github.com/sannajammeh/expo-49-test/tree/pnpm
Be sure to degit the branch pnpm and not the main.

@skurgansky-sugarcrm
Copy link

is yarn workspaces supported? I didn't manage to make it running. If i add my packages to watchFolders as this

watchFolders: [
      path.resolve(__dirname, '../../node_modules'),
      path.resolve(__dirname, '../../../node_modules'),
      path.resolve(__dirname, '..'),

it works but as i understand it avoids symlinks in node_modules and just takes it from actual location.

robhogan added a commit to robhogan/metro that referenced this issue Aug 30, 2023
Summary:
Flip the default for `unstable_enableSymlinks` to true. The option was made available in React Native 0.72 and it's been trialled by the community to the extent that we can have good confidence in it (though there are still devx gaps to guide users towards a suitable configuration).

This is also an effective prerequisite for disabling Haste/global package resolution by default in OSS, which currently allows workspaces to mostly-work in the absence of symlink support.

Closes facebook#1

Changelog:
```
 - **[Feature]**: Enable resolution through symlinks (previously experimental `unstable_enableSymlinks`)
```

Differential Revision: D48777855

fbshipit-source-id: 8da1ab84d2971736ec2560aae4a82294aec6da9f
robhogan added a commit to robhogan/metro that referenced this issue Aug 30, 2023
…book#1068)

Summary:
Pull Request resolved: facebook#1068

Flip the default for `unstable_enableSymlinks` to true. The option was made available in React Native 0.72 and it's been trialled by the community to the extent that we can have good confidence in it (though there are still devx gaps to guide users towards a suitable configuration).

This is also an effective prerequisite for disabling Haste/global package resolution by default in OSS, which currently allows workspaces to mostly-work in the absence of symlink support.

Closes facebook#1

Changelog:
```
 - **[Feature]**: Enable resolution through symlinks (previously experimental `unstable_enableSymlinks`)
```

Reviewed By: motiz88

Differential Revision: D48777855

fbshipit-source-id: a11664bac127db931c03d0e8446b6fc66e3c8a2b
@rollingmoai
Copy link

Issue closed? Any help on how I can use pnpm with unstable_enableSymlinks?

@robhogan
Copy link
Contributor

robhogan commented Aug 31, 2023

Hi folks - this issue is closed because symlinks are now supported in Metro main by default since #1068 (soon to be released in v0.79.0), and they're supported in earlier versions via resolver.unstable_enableSymlinks, first available with React Native 0.72.0.

Note that all source files and assets must be "watched" by Metro - so if you have symlinks to paths outside your project root (the directory containing your metro.config.js) you'll need to add additional paths to your watchFolders. We're working on improving error messages etc so that this is clearer. For PNPM for example, you'll want to add the directory that contains your pnpm-lock.yaml to watchFolders with something like watchFolders: [path.resolve(__dirname, '..')], (or its parent, depending on your structure).

Note also that full, non-hoisted PNPM/Yarn workspace support in React Native is still a work in progress and currently requires workarounds - not because of symlinks, but rather different package layouts that break some assumptions in Xcode and Gradle scripts, etc. We're working on that for RN 0.73.

Otherwise, please do open a new issue for any problems you run into.

@skurgansky-sugarcrm
Copy link

skurgansky-sugarcrm commented Aug 31, 2023

@robhogan Hi there, thanks for your answer.
you say that resolver.unstable_enableSymlinks: true and watchFolders: [path.resolve(__dirname, '..')] is all that required to enable symlinks for yarn workspaces?
It is not enough in my case.

I have "metro-react-native-babel-preset": "0.76.8" and RN 0.72.4
my repo structure is like this:
root
packages /
mobile (RN here)
moduleA

symlinks are created by yarn in node_modules folder of root level. And metro can't resolve them.

My metro.config.js has this settings:
resolver.unstable_enableSymlinks: true
projectRoot: __dirname,
watchFolders: [
path.resolve(__dirname, '../../node_modules'),
path.resolve(__dirname, '../../../node_modules'),
path.resolve(__dirname, '../../node_modules/@myapp/moduleA'),

 //  path.resolve(__dirname, '..'),   <- if i uncomment this line it will works. by is it working via symlinks ? i think not ?

],

@robhogan
Copy link
Contributor

@skurgansky-sugarcrm

symlinks are created by yarn in node_modules folder of root level. And metro can't resolve them.

But where does node_modules/@myApp/moduleA point to? Something like ../../../packages/moduleA, right? That's outside your watched folders, unless you watch the workspace root by uncommenting the line you've highlighted. That's why Metro can't resolve anything inside that package - you're not watching it.

To be clear, it's not sufficient to watch the symlink itself - you need to be watching the symlink target.

@skurgansky-sugarcrm
Copy link

@robhogan
root/node_modules (all npm packages and symlinks here)
root/packages/mobile (react-native here)
root/packages/moduleA (node_modules/@myApp/moduleA here)

watchFolders contains path to all npm packages and symlinks. I saw it in a bunch of articles trying to configure it. This looks strange to me. If i have to watch the path to actual packages, then why do we need unstable_enableSymlinks property and symlinks themself.

@robhogan
Copy link
Contributor

robhogan commented Sep 1, 2023

Hey @skurgansky-sugarcrm, it sounds like you might be expecting symlink support to work differently, but I'm not sure that you're facing a problem with it. There is some overlap between the functionality provided by symlinks vs Haste packages that make this a bit complicated, but symlink support is intended to provide symlink-traversing resolution similar to Node JS and other tools, as necessary for the likes of pnpm. It's not intended as a mechanism to have source outside watchFolders.

If you want to chat about the implementation and design or if you do have a specific problem, feel free to open a new issue - this one has a lot of subscribers and I'm keen to avoid noise.

@linonetwo
Copy link

linonetwo commented Sep 3, 2023

expo/expo#9591 (comment)

shamefully-hoist=true
node-linker=hoisted

in .npmrc is needed when using unstable_enableSymlinks: true,, otherwise

Unable to resolve "../../App" from "node_modules/.pnpm/expo@49.0.9_@babel+core@7.22.11/node_modules/expo/AppEntry.js"

will be thrown.

But with this config, still getting error

› Reloading apps
Error: TreeFS: Could not add directory node_modules/tiddlywiki when adding files
    at TreeFS.bulkAddOrModify (/Users/linonetwo/Desktop/repo/TidGi-Mobile/node_modules/metro-file-map/src/lib/TreeFS.js:248:17)
    at HasteMap._applyFileDelta (/Users/linonetwo/Desktop/repo/TidGi-Mobile/node_modules/metro-file-map/src/index.js:691:16)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async /Users/linonetwo/Desktop/repo/TidGi-Mobile/node_modules/metro-file-map/src/index.js:327:9
    at async DependencyGraph.ready (/Users/linonetwo/Desktop/repo/TidGi-Mobile/node_modules/metro/src/node-haste/DependencyGraph.js:95:5)
    at async Bundler.ready (/Users/linonetwo/Desktop/repo/TidGi-Mobile/node_modules/metro/src/Bundler.js:69:5)
    at async IncrementalBundler.ready (/Users/linonetwo/Desktop/repo/TidGi-Mobile/node_modules/metro/src/IncrementalBundler.js:289:5)
    at async Server.ready (/Users/linonetwo/Desktop/repo/TidGi-Mobile/node_modules/metro/src/Server.js:1085:5)

uninstall tiddlywiki fix the problem.

linonetwo added a commit to tiddly-gittly/TidGi-Mobile that referenced this issue Sep 3, 2023
@cameronelliott
Copy link

cameronelliott commented Sep 26, 2023

This was very time consuming for me to debug and figure out why symlinks were not working.
I spent a lot of time massaging custom dependency package.jsons to figure out why there were broken. (they were not)
Finally started using the 'ms' package only to discover it's not an error in my packages.
Started thinking about giving up on RNative support.

For others who might be having similar issues:

  • Prior to metro 79.0 you need the unstable_enableSymlinks in your metro.config.ms
  • You also must have any targets of any symlinks covered by the watchFolders section of the config also.
  • In my case relative paths were not working, but absolute paths seemed to do the trick.

Most of this was gleaned from here: #1 (comment)

const config = {

   watchFolders: [
        '/Users/c/Documents/foodir/ms',
    ],

    resolver: {
       unstable_enableSymlinks: true
    }
};

@karlhorky
Copy link

For those on Expo SDK 50 + RN 0.73 + pnpm trying to use the new symlinks support (without node-linker=hoisted or other workarounds), it seems there are other file resolution issues with Expo too:

@lbxa
Copy link

lbxa commented Oct 13, 2024

@karlhorky I managed to get this working here pnpm/pnpm#4286 (comment).

@karlhorky
Copy link

@lbxa nice, thanks for spending the time documenting that!

@karlhorky
Copy link

For official first-class support in Expo, it looks like non-hoisted pnpm support is scheduled for Expo SDK 52 - mentioned in @byCedric's post in the pnpm repo:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet