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

Find all symlinks in (scoped) node_modules and merge root flag #12400

Closed
wants to merge 1 commit into from

Conversation

Swaagie
Copy link

@Swaagie Swaagie commented Feb 15, 2017

Motivation

Improve DX by finding all symlinks in node_modules. Both @Scoped modules and discovered node_modules from symlinks. No more than in RN can exist in the file structure, so allow discovering symlinks through the root flag.

Improvements

tldr; symlinking through yarn link or npm link works for all modules and Watchman can be installed.

Scoped modules

Symlinks were only discovered for a single level of node_modules, however scoped modules exist two level deeps, e.g node_modules/@scoped/module. If the first character of a module folder is @ that folder is also read for possible symlinks. tested with jest

app
├── node_modules
│   ├── @scoped         // scoped, will be crawled
│   │   ├──  module     // if symlink will be added as source
│   ├── regular
├── lib
├── package.json

Recursive detection of symlinked modules

A symlinked module is likely to have node_modules installed. When developing highly modularized apps, another symlink in the symlinked module node_modules can exists. Whenever a symlink is found the /resolved/path/to/symlink/node_modules/ will again be read for potential symlinks. tested with jest

app
├── node_modules
│   ├── symlink         // symlink added to source and crawled 
│   ├── react-native 
├── lib
├── package.json
symlink
├── node_modules
│   ├── another         // will also be found and added to source
another
├── node_modules
│   ├── required

Configurable and crawled root flag for RN packager

Due to the nature of @providesModule no more than a single RN package can ever exist in the crawled and read file structure. RN is often a devDependency for modules, which works fine with regular installs. However, symlinking will often lead to multiple RN packages.

Removing these packages each time you work/change symlinks is cumbersome. To work efficient and effective with symlinks, RN itself has to be symlinkable as well. node_modules are resolved relative to the source of RN. The root flag is a feature that could potentially solve this problem. However, currently it is ignored for the packager server, see this change.

Adding root back in with args.projectRoots and reading its node_modules allows RN to exists outside the app file structure and create symlinks from each linked module back to RN.

app
├── node_modules
│   ├── symlink            // symlink added to source and crawled 
│   ├── react-native       // symlink added to source and crawled 
├── lib
├── package.json
symlink
├── node_modules
│   ├── react-native       // devDependency that can now be symlinked
react-native               // cloned from React-native repo or installed.

From the app itself the following two commands can now be run:

react-native start -- --root .
react-native run-ios

Passing the . working directory allows the packager to pickup on the actual source.

Node-haste should resolve symlinks when reading a directory.

A required module from a symlinked folder/file will return the symlinked directory as part of its path, so node-haste should resolve symlinked directories before actually trying to require a file or module.

Performance

Crawling additional directories in a complex setup with 4 (nested) symlinks and multiple node_modules directories being crawled barely changes startup time, e.g:

Scanning 1820 folders for symlinks in /projects/godaddy/module/node_modules (20ms)

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Feb 15, 2017
@Swaagie
Copy link
Author

Swaagie commented Feb 15, 2017

Unsure why circleci failed, if someone could provide me with suggestions on how to fix the issue I'll look into it.

@ericvicenti
Copy link
Contributor

This is pretty large change that is likely to conflict with other improvements happening within the packager. Symlink support has been a top-requested feature for a long time, and the packager team is definitely interested in addressing the problem. Unfortunately I'm not sure if this proposal is the best way to do it, and without the support of the core team, I think it is unlikely to be accepted.

@indexzero
Copy link

@ericvicenti @hramos following up from our conversation at ReactConf. Based on some anecdotal testing this has zero affect on Packager startup time:

On latest react-native

Scanning 561 folders for symlinks in /REDACTED/path/to/our/project/name/node_modules (15ms)

Against this branch

Scanning 678 folders for symlinks in /REDACTED/path/to/our/project/name/node_modules (8ms)

since Facebook does not use symlinks and Packager startup time is not affected we're hoping that this can get merged. Totally understand if we are going to wait for @cpojer to come back from vacation though 👍

@alexhanson
Copy link

Should we close this pull request given that #12435 was accepted in commit c97c1e5?

@indexzero
Copy link

@alexhanson the fix related to scoped modules is not in #12435 so I believe this still has merits on its own. Will touch base with @Swaagie about getting this rebased off master since there does appear to be some overlap.

@Swaagie
Copy link
Author

Swaagie commented Apr 3, 2017

@alexhanson @indexzero as far as I can tell these two PR's are still covering distinct issues. #12435 is about having more robust root discovery. While this PR covers other features, e.g. symlink discovery and the ability to configure the root folder through startup arguments.

I've taken another look at the code that resolves the root of the project and I think both PR's complement each other. That said, I'd like to stay open minded to change and/or remove any dead or duplicate code. Please point me out if I'm missing something obvious.

@kcjonson
Copy link

Whats the status on this one? We're still running into issues with symlinks.

@cpojer
Copy link
Contributor

cpojer commented Jun 8, 2017

Metro is now in a separate repository. Would you mind sending the same pull request to the metro-bundler repo? https://github.com/facebook/metro-bundler

Thank you!

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.

8 participants