Bugfix 3724 -- Make --modules-folder more functional
#3792
Conversation
This is a first pass on teaching `check` to utilize the modules-folder option. Later commits include a more complete refactor.
This commit fixes linking bins so that it pays attention to the `--modules-folder` option. Test included.
This commit modifies the run command so that it will look in the `--modules-folder` bin if provided.
The existing logic of `verifyTreeCheck` was a bit hard to follow and
it was doing more than necessary.
This commit reworks this function in a few ways:
* Clarifies intent that a missing package.json is indeed supposed
to indicate "matches any required version"
* DRYs up some repeated code
* Stops double-checking the existence of directories, refactoring
instead into a more standard DFS
* Stops caching hits based upon the version being searched for,
which could be a semver string, and would thus have a lot of
misses. Instead, key only on the directory being searched, and
cache the version string of the found package.
This is the biggest change overall: looking at the code, it seems
like there would be a lot of misses and re-fetches. Not that it
was slow, but while I'm here... It halved the number of fstat
checks on my machine, running over the `yarn` dependencies
themselves.
twooster
force-pushed
the
bugfix-3724
branch
2 times, most recently
from
bb83d7f to
3599292
Compare
src/package-linker.js
Outdated
| @@ -329,7 +329,7 @@ export default class PackageLinker { | |||
| topLevelDependencies, | |||
| async ([dest, {pkg}]) => { | |||
| if (pkg.bin && Object.keys(pkg.bin).length) { | |||
| const binLoc = path.join(this.config.cwd, this.config.getFolder(pkg)); | |||
| const binLoc = this.config.modulesFolder || path.join(this.config.cwd, this.config.getFolder(pkg)); | |||
There was a problem hiding this comment.
maybe config.getFolder should have this condition instead?
|
Thanks for the PR, @twooster! |
|
Yup, I see that. Will get to it hopefully tomorrow. I was having some spurious unrelated failures on my machine, so I punted it up to Circle to see what happened. |
|
@twooster any updates? We have this issue too and it's important for us basically for using yarn in containers too. |
|
|
||
| let modulesFolder; | ||
| if (flags.modulesFolder) { | ||
| modulesFolder = flags.modulesFolder = path.join(cwd, flags.modulesFolder); |
There was a problem hiding this comment.
This is not safe since the argument can be any path, not just a relative path. I think you can simplify and fix this part as follows:
// Up above
const DEFAULT_MODULES_FOLDER = './node_modules';
//...
const modulesFolder = path.resolve(flags.modulesFolder || DEFAULT_MODULES_FOLDER);
What do you think? (See https://nodejs.org/api/path.html#path_path_resolve_paths for path.resolve() docs)
Note: This also implies replacing all 'node_modules' usages with the new DEFAULT_MODULES_FOLDER constant
There was a problem hiding this comment.
@BYK I've pulled the actual fork and working on it, would you mean (as cwd in the context directory):
const modulesFolder = path.resolve(cwd, flags.modulesFolder || DEFAULT_MODULES_FOLDER);There was a problem hiding this comment.
That looks exactly like my code example above. Did you change something and I'm missing it? :(
There was a problem hiding this comment.
Yup, added cwd as 1st arg, subtle change.
| @@ -8,8 +8,8 @@ const request = require('request'); | |||
| const path = require('path'); | |||
| import {runInstall} from '../_helpers.js'; | |||
|
|
|||
| async function linkAt(config, ...relativePath): Promise<string> { | |||
| const joinedPath = path.join(config.cwd, ...relativePath); | |||
| async function linkAt(dir, ...relativePath): Promise<string> { | |||
There was a problem hiding this comment.
May be we should put modulesDir into config and go along with that across the whole project?
| version, | ||
| }); | ||
| } | ||
| pushDependencies(rootManifest.dependencies); |
| if (isLinkedDepencency) { | ||
| continue; | ||
|
|
||
| function pushDependencies(dependencies: ?Dependencies) { |
There was a problem hiding this comment.
A bit more "functional" version (ideal version would use a .filter().map() combo but we know that'd be less efficient in JS):
function pushDependencies(dependencies: ?Dependencies) {
(dependencies || []).forEach(name => {
const version = ....
})
}
| dependenciesToCheckVersion.push({ | ||
| name, | ||
| key: name, | ||
| searchPath: [rootModulesFolder], |
There was a problem hiding this comment.
Looks like you can pull [rootModulesFolder] part out since it never changes?
| } | ||
|
|
||
| const locationsVisited: Set<string> = new Set(); | ||
| const missing = false; |
There was a problem hiding this comment.
Why are we using constants instead of booleans?
| reportError('packageWrongVersion', dep.originalKey, dep.version, pkg.version); | ||
| continue; | ||
| const searchPath = dep.searchPath; | ||
| let version: string | boolean = false; |
There was a problem hiding this comment.
May be use null to indicate a lack of version instead of false?
src/cli/commands/check.js
Outdated
|
|
||
| if (pkgVersions.has(manifestLoc)) { | ||
| version = pkgVersions.get(manifestLoc) || missing; | ||
| if (version !== missing) { // found |
There was a problem hiding this comment.
This part can become:
version = plgVersions.get(manifestLoc);
if (!version) {
break;
}
| break; | ||
| } | ||
| } | ||
| searchPath.pop(); |
There was a problem hiding this comment.
Kind of hard to follow this logic. May be can use some comments. Also do we really need all this refactoring? If not let's defer that to a follow-up diff to see exactly what logic changes are needed to make this patch work.
| @@ -39,108 +40,90 @@ export async function verifyTreeCheck( | |||
| const registryName = 'yarn'; | |||
| const registry = config.registries[registryName]; | |||
| const rootManifest = await config.readManifest(registry.cwd, registryName); | |||
| const rootModulesFolder = config.modulesFolder || path.join(registry.cwd, registry.folder); | |||
There was a problem hiding this comment.
I think we should just make condfig.modulesFolder to hold the ultimate truth about this.
|
@twooster - need any help from us? |
|
@twooster hello again! Don't wanna feel like pressuring you but I think this PR is awesome and there's little way to go to get it merged. Please let us know if you need anything from us or you don't have time to keep working on this so we can set time aside to finish this up :) |
|
@BYK Would be great if you could finish this, looks like some time has been passed since the last response. |
That was my intention but it requires a bit more time than I currently have. If you're interested, I can help. I already reviewed the original code. It needs a merge from master which should be clean now since I did it earlier and Github doesn't report new conflicts. |
I can't really help you here Facebook, Inc. I am just a user of your software. |
|
Hey @schmunk42, I'm not a Facebook employee, but that's an inappropriate thing to say to people who are. This is an open source project and not a product of Facebook. Facebook isn't even on the license. We're all in this together, so if you want to improve the software you use, you are encouraged to. Otherwise do not place an additional burden on maintainers. If you or someone else wants to take this through the finish line, we'd all encourage you to. Otherwise this will have to be closed since contributors are hard at work on many many other things. Please be kinder to the people who are providing you with free software |
|
@schmunk42 @thejameskyle @BYK I've pulled the current PR, I give a try to take into account review feedbacks and make this one merged ASAP 😉 in another PR I will point here soon. PS: (I'm not a Facebook employee too ^^). That said, not sure that @schmunk42 was an offense, I believe in people kindness & generosity, if effectively makes happen projects like yarn 👼. FOSS is not a peaceful journey BTW. |
|
Is anyone still working on this? If so, would anyone like assistance getting it done? Looks close and I'm starting to see workarounds (not a good thing). To those who have started contributing: please acknowledge and let everyone know how it's going. No one wants to duplicate effort, so if you just don't have the time to wrap this up soon it's helpful to be public about it. No shame! Life and work are real. Unblocking contributions is itself a contribution. |
|
This will be great! As npm has many ways of altering the location of /var/www# export NPM_CONFIG_PREFIX=$PWD/.node_modules_deb
/var/www# yarn config list
yarn config v1.1.0
info yarn config
{ 'version-tag-prefix': 'v',
'version-git-tag': true,
'version-git-sign': false,
'version-git-message': 'v%s',
'init-version': '1.0.0',
'init-license': 'MIT',
'save-prefix': '^',
'ignore-scripts': false,
'ignore-optional': false,
registry: 'https://registry.yarnpkg.com',
'strict-ssl': true,
'user-agent': 'yarn/1.1.0 npm/? node/v8.6.0 linux x64',
lastUpdateCheck: 1507853955885 }
info npm config
{ prefix: '/var/www/.node_modules_deb' }
Done in 0.09s.Looking forward to |
|
I'll go ahead and close this PR due to lack of any movement recently. It is already out of date and a fresh start may be easier at this point to get this issue fixed. If anyone disagrees, feel free to comment here and ping me. |
|
This error is annoying :( |
|
Hey @BYK - would you accept PRs that build off this one here? This issue continues to impact a project I'm working on and it would be nice to get it resolved. |
Summary
The
--modules-folderflag seems to be a bit broken, seen in #3724 -- bin links were not working. Additionally, the semantics of what it means to have this flag, and where it should be usable, is a bit unclear.This commit doesn't address the later, but does attempt to address the former. Included:
installare now working correctlyrunnow looks in the.binsubfolder if--modules-folderis providedchecknow checks in--modules-folder, if providedMy personal motivation is to be able to move my
node_modulesfolder outside of my application for the purposes of having a clean Docker environment when volume-mounting back to host. No more files with odd UIDs, etc. It seems to work anyway if you specifyNODE_PATH, etc.Test plan
Tests for the binlinks is included. For the rest, I wanted to get feedback on the intent of
--modules-folder, to ensure I properly understand it, before writing actual tests.For my purposes, I tested by self-yarning yarn with a modules folder outside of the main path. Two folders,
yarn/, where my updated code is, andyarn-test/, which is another instance. Bootstrapping one from the other and playing around:As you can see, the syntax for
runis still a bit confusing, given that you have to specify the NODE_PATH for it to work, so maybe there's an improvement there. Happy to hear any suggestions and have a discussion.