Symlink support for packager

[Kureev]

I saw we have quite a few user requests for symlink support...

Test plan (required)

1. Create a symlink in node_modules (for instance use npm link)
2. Run npm start
3. Profit!

Code formatting

Look around. Match the style of the rest of the codebase. See also the simple style guide.

For more info, see the "Pull Requests" section of our "Contributing" guidelines.

[ghost]

By analyzing the blame information on this pull request, we identified @philikon and @christopherdro to be potential reviewers.

[bestander]

Cc @matryoshcow, our new Packager man in RN team

[bestander]

FYI, big changes will be coming to packager, so all changes to current one need to be considered

[matryoshcow]

@Kureev, this is pretty neat! A couple of thoughts:

1. as it stands, this would be enabled by default, even though it's clearly targeted at specific class of users; it might be a good idea to make symlink support optional
2. to the extent possible, we're using async I/O; could your solution be modified in this manner?

bonus nitpick: one-letter variable names are the reason why we can't have nice things :)

[Kureev]

Hi @matryoshcow!

About your notes:

1. Yeah, it's definitely not for everybody, but on the other hand the overhead you have is just about fs.readdir + fs.lstat. You can already specify --projectRoots and it'll work for you. My intention was to make it work "by default"
2. Sure!

[mkonicek]

@matryoshcow If I have no symlinks in node_modules, this feature is disabled. This way the feature is only enabled for people who want to use it.

It might happen that people have symlinks in node_modules without knowing about it though, which could be confusing. Printing info about the found symlinks on packager startup could help.

Making this optional with a packager argument would work too, but people would have to learn about the extra argument (Google -> StackOverflow etc., figuring out if info is up to date which is extra work everyone has to do), and what if people launch the packager using Xcode? There are several different ways people launch the packager which makes it more work to make the feature super easy to use.

[mkonicek]

I'll request changes just for adding some debugging output so people know they're using this feature.

Agree with @matryoshcow's nit that some variable names could be made a little bit clearer, for example:

.filter(...)
.map(symlink => path.resolve(process.cwd(), fs.readlinkSync(symlink)));

[mkonicek]

In short I agree with @Kureev that making this "just work" is a nice developer experience, compared to having to figure out how to enable the feature.

[bestander]

A concern that an extra walk through the node_modules folder slows down
packager start.
Any extra IO adds up especially in this case when it is doing "native" node
fs operations that are prone to throw ENOENT.

On Wed, Jul 27, 2016 at 1:05 PM, Martin Konicek [email protected]
wrote:

@matryoshcow https://github.com/matryoshcow If I have no symlinks in
node_modules, this feature is disabled. This way the feature is only
enabled for people who want to use it.

It might happen that people have symlinks in node_modules without knowing
about it though, which could be confusing. Printing info about the found
symlinks on packager startup could help.

Making this optional with a packager argument would work too, but people
would have to learn about the extra argument (Google -> StackOverflow etc.,
figuring out if info is up to date, extra work), and what if people launch
the packager using Xcode? There are several different ways people launch
the packager which makes it more work to make the feature super easy to use.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#9009 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACBdWA6BpFv9672eJzKu2-VmZWV09cEkks5qZ0mBgaJpZM4JUblt
.

[bestander]

This means 750 sync lstat operations during packager start.

[matryoshcow]

@mkonicek hmm... agreed - making this "just work" is a good idea, as long as it doesn't slow down the startup time. Since we're only talking about node_modules here the overhead may be negligible even for us, but let's confirm this before we commit to patching it in.

[matryoshcow]

@Kureev let's add this under a flag to begin with and perhaps log the symlink resolution time; we can enable it by default once we've observed it for a bit.

[mkonicek]

Right, readdirSync is not recursive so this should be fast but agree adding the feature behind a flag first is a good way to introduce it. Good idea with logging the time!

EDIT: Oh wait, internally at fb there are a lot of folders in node_modules (when starting the packager from the RN folder). Good point @bestander about this being 750 sync checks. With npm 2 I only have two folders in my app's node_modules: react and react-native, I was thinking of that use case :D

[ryanlinnane]

What's the current status of this? I found that via using npm link, watchman broke because it referenced a symlink to a symlink. fs.realpathSync did the trick. Moreover, I found that the path construction sometimes was a directory off, so changing the base folder to lookupFolder fixed up a few issues.

Code:

module.exports = function findSymlinksPaths(lookupFolder) {
  const timeStart = Date.now();
  const folders = fs.readdirSync(lookupFolder);
  const resolvedSymlinks = folders.map(folder => path.resolve(lookupFolder, folder))
    .filter(folderPath => fs.lstatSync(folderPath).isSymbolicLink())
    .map(symlink => fs.realpathSync(path.resolve(lookupFolder, fs.readlinkSync(symlink))));
  const timeEnd = Date.now();
  console.log(`Scanning ${folders.length} folders for symlinks in ${lookupFolder} (${timeEnd - timeStart}ms)`);
  return resolvedSymlinks;
};

[aleclarson]

@ryanlinnane Check out #9792 and let me know if you still have issues.

[cpojer]

I agree we should figure out a path forward to support this both in RNP and in Jest. Give us some time to get our house in order please, as @bestander pointed out we are planning some pretty significant changes.