-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Conversation
Unsure why circleci failed, if someone could provide me with suggestions on how to fix the issue I'll look into it. |
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. |
@ericvicenti @hramos following up from our conversation at ReactConf. Based on some anecdotal testing this has zero affect on Packager startup time: On latest
Against this branch
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 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. |
@alexhanson @indexzero as far as I can tell these two PR's are still covering distinct issues. #12435 is about having more robust I've taken another look at the code that resolves the |
Whats the status on this one? We're still running into issues with symlinks. |
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! |
Motivation
Improve DX by finding all symlinks in
node_modules
. Both @Scoped modules and discoverednode_modules
from symlinks. No more than in RN can exist in the file structure, so allow discovering symlinks through theroot
flag.Improvements
tldr; symlinking through
yarn link
ornpm 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.gnode_modules/@scoped/module
. If the first character of a module folder is@
that folder is also read for possible symlinks. tested with jestRecursive detection of symlinked modules
A symlinked module is likely to have
node_modules
installed. When developing highly modularized apps, another symlink in the symlinked modulenode_modules
can exists. Whenever a symlink is found the/resolved/path/to/symlink/node_modules/
will again be read for potential symlinks. tested with jestConfigurable and crawled
root
flag for RN packagerDue 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 adevDependency
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. Theroot
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 withargs.projectRoots
and reading itsnode_modules
allows RN to exists outside the app file structure and create symlinks from each linked module back to RN.From the app itself the following two commands can now be run:
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: