-
Notifications
You must be signed in to change notification settings - Fork 393
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
feat: use SDR for conflict detect perf improvements #3246
Conversation
this.sourceComponents = this.isManifest | ||
? await ComponentSet.fromManifest({ | ||
manifestPath: this.componentPath, | ||
resolveSourcePaths: [this.projectPath] |
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 think we need to make this a list of the project's package directories. If projectPath
is the root of the project, it'll still work but will end up over-scanning irrelevant files like node_modules which could impact the performance
getRootWorkspacePath(), | ||
this.isManifest | ||
); | ||
return await this.cacheService.getSourceComponents(); |
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.
Nit, remove await
since we're already returning a promise
if (throwErrorOnFailure) { | ||
throw error; | ||
} | ||
private async handleCacheResults( |
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.
Where is this handleCacheResults used and how is this different from the one in forceSourceDiff.ts ?
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.
Never mind, so this is specifically used for the conflict detection part here. I think we need a separate diffService.ts and move some of the diff specific code there. Can be part of a separate devchoice/refactoring story.
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. There is some overlap, but they will diverge as conflict detection progresses.
What does this PR do?
What issues does this PR fix or reference?
@W-9164033@