-
Notifications
You must be signed in to change notification settings - Fork 593
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
fix: support subspace in package-extractor #4655
fix: support subspace in package-extractor #4655
Conversation
const queue: RushConfigurationProject[] = [project]; | ||
|
||
while (queue.length > 0) { | ||
const _project: RushConfigurationProject = queue.shift()!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we forget to fill 'projects' in this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, deleted by mistake...
@@ -483,8 +499,13 @@ export class PackageExtractor { | |||
path.join(packageJsonRealFolderPath, 'package.json') | |||
); | |||
|
|||
const targetSubspace: IExtractorSubspace | undefined = subspaces?.find( | |||
(p) => p.pnpmInstallFolder && Path.isUnder(packageJsonFolderPath, p.pnpmInstallFolder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious: would it be possible to have multiple "subspaces" here?
It's related if there are any nested projects in a monorepo (which is not recommended)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, because each subspace's pnpmInstallFolder will be located at common/temp and they won't be nested right?
We can't have multiple subspaces in following logic, or it will have some issues
One dependency from one subspace might be resolved to another subspace, which is not expected
try { | ||
// The PNPM virtual store links are created in this folder. We will resolve the current package | ||
// from that location and collect any additional links encountered along the way. | ||
// TODO: This can be configured via NPMRC. We should support that. | ||
const pnpmDotFolderPath: string = path.join(pnpmInstallFolder, 'node_modules', '.pnpm'); | ||
const pnpmDotFolderPath: string = path.join( | ||
targetSubspace.pnpmInstallFolder!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we remove the "!" sign here? Instead, adding it to if-condition
if (targetSubspace && targetSubspace.pnpmInstallFolder) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
*/ | ||
pnpmInstallFolder?: string; | ||
subspaces?: IExtractorSubspace[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: It's a breaking change to remove "pnpmInstallFolder" and entirely replace it with "subspaces". If we do so, it's a major version bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, to avoid breaking API compatibility we could allow both alternatives: Either you specify IExtractorState.transformPackageJson
etc, or else you can specify IExtractorState.subspaces
(but not both at the same time).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will rethink how can we make API more compatible here.
const queue: RushConfigurationProject[] = [project]; | ||
|
||
while (queue.length > 0) { | ||
const _project: RushConfigurationProject = queue.shift()!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
const _project: RushConfigurationProject = queue.shift()!; | ||
|
||
for (const dependency of _project.dependencyProjects) { | ||
queue.push(dependency); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder visited projects should be ignored.
To illustrate:
const visited: Set<RushConfigurationProject> = new Set();
...
for (const dependency of _project.dependencyProjects) {
if (visited.has(dependency) {
continue;
}
...
visited.add(dependency);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it would have some performance improvement.
if (!scenarioConfiguration.json.omitPnpmWorkaroundLinks) { | ||
subspace.pnpmInstallFolder = project.subspace.getSubspaceTempFolder(); | ||
} | ||
subspaces.push(subspace); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming A, B lives in the same subspace. It feels like the current logic generates the same subspace twice. Am I understanding this correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, I might change it to a Set to avoid that case.
We don't need an official build test necessarily, but I'd really like to at least see a branch that illustrates how this algorithm is used. If you can't easily reproduce the error being fixed, that's okay, but it would be very helpful to have an example |
|
||
while (queue.length > 0) { | ||
const _project: RushConfigurationProject = queue.shift()!; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shift()
is an O(n)
operation making this loop O(n^2)
. If N=1,000 then N^2=1,000,000 which might be significant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, I think I can simple convert it to be a DFS by replacing shift with pop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
@octogonz I have added the example rush repo and a screenshot of deployment output. |
40edee8
to
7228c25
Compare
William also created a test branch for us here: I had a long day today at the conference, so I will test it first thing tomorrow morning. Mainly I want to inspect the symlinks created by |
Yep, I'm also using the repo William created, I put its link in this PR description. |
@@ -483,8 +499,13 @@ export class PackageExtractor { | |||
path.join(packageJsonRealFolderPath, 'package.json') | |||
); | |||
|
|||
const targetSubspace: IExtractorSubspace | undefined = subspaces?.find( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This find()
is calling Path.isUnder()
for every subspace (including paths that don't match), for every node_modules package that gets crawled. If we have n=1000
subspaces and n=1000
installed folders, that is O(n*m)=O(1,000,000)
operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a couple examples of how Rush handles these problems:
- LookupByPath.ts constructs an efficient lookup tree; OR
- PackageJsonLookup simply caches the results.
@EscapeB @william2958 Because PR #4655 is an important fix, we don't need to address the problem in this PR. But I think we should consider optimizing these lookups because O(n^2)
loops can and do cause noticeable slowness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK,will try to improve and that in later PR.
// Transform packageJson using the provided transformer, if requested | ||
const packageJson: IPackageJson = transformPackageJson?.(originalPackageJson) ?? originalPackageJson; | ||
const packageJson: IPackageJson = | ||
targetSubspace?.transformPackageJson?.(originalPackageJson) ?? originalPackageJson; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API design is confusing for users because it ignores mistakes. In the below example, what is the pnpmInstallFolder
for the default
subspace? It is unclear.
this.extractAsync({
pnpmInstallFolder: 'abc',
subspaces: [
{ subspaceName: 'default', pnpmInstallFolder: 'def' },
{ subspaceName:'other', pnpmInstallFolder: 'ghi' }
]
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'm working on a fix)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -36,6 +36,7 @@ export interface IExtractorOptions { | |||
pnpmInstallFolder?: string; | |||
projectConfigurations: IExtractorProjectConfiguration[]; | |||
sourceRootFolder: string; | |||
subspaces?: IExtractorSubspace[]; | |||
targetRootFolder: string; | |||
terminal: ITerminal; | |||
transformPackageJson?: (packageJson: IPackageJson) => IPackageJson | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
transformPackageJson?: (packageJson: IPackageJson) => IPackageJson | undefined; | |
transformPackageJson?: (packageJson: IPackageJson) => IPackageJson; |
I think this was just typed incorrectly. I don't see any usage of this transform that could reasonably need the method to return "undefined".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I checked and PNPM itself seems to reject that case:
if (pnpmfile?.hooks?.readPackage) {
const readPackage = pnpmfile.hooks.readPackage;
pnpmfile.hooks.readPackage = async function(pkg, ...args2) {
pkg.dependencies = pkg.dependencies ?? {};
pkg.devDependencies = pkg.devDependencies ?? {};
pkg.optionalDependencies = pkg.optionalDependencies ?? {};
pkg.peerDependencies = pkg.peerDependencies ?? {};
const newPkg = await readPackage(pkg, ...args2);
if (!newPkg) {
throw new BadReadPackageHookError(pnpmFilePath, "readPackage hook did not return a package manifest object.");
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
* Can't be used with subspaces, if subspaces field is specified, we will use pnpmInstallFolder configured | ||
* for each subspace and ignore this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update this to throw if both are provided instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in my EscapeB#1
targetSubspace?.transformPackageJson?.(originalPackageJson) ?? | ||
transformPackageJson?.(originalPackageJson) ?? | ||
originalPackageJson; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should validate that only one of the two options (subspaces
vs top-level transformPackageJson
/pnpmInstallFolder
) is provided, and throw an error if it's not the case. We wouldn't want to hide why the top-level ones are ignored when someone specified subspaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in my EscapeB#1
Summary
Fix issue that can't extract projects dependencies correctly if there exist multiple subspaces.
Details
Now only subspace that for entry project will be passed to package extractor, but if there are multiple projects belongs to different subspaces, dependencies in other subspaces can't be extract correctly.
How it was tested
Tested with in-house production project.
Also there is an example that shows what the file structure will be like after this PR merged.
![image](https://private-user-images.githubusercontent.com/22115706/323465930-8877436d-d671-42cc-8c98-15ee02d47a8d.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQ3MDQ0NjMsIm5iZiI6MTcyNDcwNDE2MywicGF0aCI6Ii8yMjExNTcwNi8zMjM0NjU5MzAtODg3NzQzNmQtZDY3MS00MmNjLThjOTgtMTVlZTAyZDQ3YThkLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODI2VDIwMjkyM1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTFhM2IwM2E5ZGExNWQ5MThjNTQ5Y2VkNWI3N2MzY2NlMzM2YzI0OGUxYmEwZTRiN2ZmZWI3NWU0MWM3MjIyMGEmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.5VSsvDIi5Hak15lykvSh4oZXpm3T_0gGXiaviDxyngM)
Here is the example subspace repo https://github.com/william2958/rush-subspace-demo
Impacted documentation
The internal API for PackageExtractor has been changed to support multiple subspaces