Forces workspaces to always be installed from their root #4030
Conversation
|
New tests? :) |
They'll come in time, before merge 😛 - I wanted some eyes on the PR while I'm working on them. |
|
Added a test |
| await reInstall.init(); | ||
|
|
||
| expect((await fs.readFile(`${config.cwd}/yarn.lock`)).indexOf(`"b@file:b"`)).not.toEqual(-1); | ||
| expect((await fs.readFile(`${config.cwd}/yarn.lock`)).indexOf(`"a@file:./a"`)).not.toEqual(-1); |
There was a problem hiding this comment.
Awoid reading the same file twice and store the file contents in a variable, please.
src/util/git.js
Outdated
| @@ -267,7 +267,7 @@ export default class Git implements GitRefResolvingInterface { | |||
| if (await fs.exists(cwd)) { | |||
| await spawnGit(['pull'], {cwd}); | |||
| } else { | |||
| await spawnGit(['clone', gitUrl.repository, cwd]); | |||
| await spawnGit(['clone', gitUrl.repository, cwd], {cwd: this.config.cwd}); | |||
src/util/normalize-manifest/fix.js
Outdated
| @@ -306,4 +306,10 @@ export default (async function( | |||
| info.license = inferredLicense; | |||
| } | |||
| } | |||
|
|
|||
| for (const hint of ['dependencies', 'devDependencies', 'optionalDependencies', 'peerDependencies']) { | |||
There was a problem hiding this comment.
Didn't we have this in somewhere else? Somewhere more central?
| // It won't work if we don't yet know what's the folder we'll use as root. It's not a | ||
| // big deal tho, because it only happens when trying to figure out the root, and we | ||
| // don't need to know the dependencies / devDependencies at this time. | ||
| if (!lockfileFolder) { |
There was a problem hiding this comment.
lockfileFolder can only be an empty string since you define its type as string. Is this check really necessary?
| return; | ||
| } | ||
|
|
||
| for (const hint of ['dependencies', 'devDependencies', 'optionalDependencies', 'peerDependencies']) { |
There was a problem hiding this comment.
Can you move the ['dependencies', 'devDependencies', 'optionalDependencies', 'peerDependencies'] part into a constant so we don't have to create this array ever again in anywhere in the app?
| } | ||
|
|
||
| for (const hint of ['dependencies', 'devDependencies', 'optionalDependencies', 'peerDependencies']) { | ||
| if (!info[hint]) { |
There was a problem hiding this comment.
hint is not a great name. How about dependencyType?
| prefix = LINK_PROTOCOL_PREFIX; | ||
| } | ||
|
|
||
| if (prefix) { |
There was a problem hiding this comment.
Early exit:
if (!prefix) {
return;
}
There was a problem hiding this comment.
No, this has to be executed for each dependency. continue would be the right keyword, but it would behave no differently than it currently does.
There was a problem hiding this comment.
Sorry, I was going for continue. It makes the code easier to follow and takes a level of indent.
There was a problem hiding this comment.
It makes the code easier to follow and takes a level of indent.
That's kinda subjective :)
There was a problem hiding this comment.
Taking a level of indent, making the code closer to the left are both facts. I'd argue text closer to left side is easier to read text indented otherwise we'd have large margins on the left side for all text. That said I'm gonna stop here :D
|
|
||
| if (hasPrefix) { | ||
| // If the original value was using the "./" prefix, then we output a similar path. | ||
| // We need to do this because otherwise it would cause problems with already existing |
There was a problem hiding this comment.
Why not normalize everything in the lockfile so we don't need this? Also I have concerns around the regular expression above and Windows. May be we should use something else?
There was a problem hiding this comment.
This commit is to be shipped in patch version, so I'd prefer not to change the lockfile output. Apart from that, yeah, this code path could be removed in the next minor.
| // If the original value was using the "./" prefix, then we output a similar path. | ||
| // We need to do this because otherwise it would cause problems with already existing | ||
| // lockfile, which would see some of their entries being unrecognized. | ||
| relativeTarget = relativeTarget.replace(/^(?!\.{0,2}\/)/, `./`); |
There was a problem hiding this comment.
No idea what this is doing but I'm guessing it can be replaced with path.normalize() or something similar?
There was a problem hiding this comment.
If the dependency is link:./foo, we need it to be link:./foo after this code. If it is link:foo, we need it to be link:foo. If we don't, we change the output of the lockfile, and it might cause lockfiles to change. Normalize doesn't do the job, because we actually need to do the opposite of normalizing :)
There was a problem hiding this comment.
Ah! Well, may be add a todo comment to follow up with this then?
| // It won't work if we don't yet know what's the folder we'll use as root. It's not a | ||
| // big deal tho, because it only happens when trying to figure out the root, and we | ||
| // don't need to know the dependencies / devDependencies at this time. | ||
| if (!lockfileFolder) { |
| await reInstall.init(); | ||
|
|
||
| expect((await fs.readFile(`${config.cwd}/yarn.lock`)).indexOf(`"b@file:b"`)).not.toEqual(-1); | ||
| expect((await fs.readFile(`${config.cwd}/yarn.lock`)).indexOf(`"a@file:./a"`)).not.toEqual(-1); |
This PR fixes #4028.
When installing, we will now always start installing from the root of the workspace, or the cwd if there's none (which is what defines
lockfileFolder).The relative paths are now resolved when reading the json files rather than when resolving them. It allows us to factor the logic, and I also think it also solves a pair of issues where relative subdependencies were not correctly resolved when installed from non-
link:and non-file:dependencies (this is only my belief based on my lecture of tbe source code, I haven't yet took the time to validate this hypothesis).I still have to write integration tests, but the testcase linked with #4028 is fixed, and the regular testsuite is still green.