-
Notifications
You must be signed in to change notification settings - Fork 932
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: store dependencies graph in component model #9214
base: master
Are you sure you want to change the base?
Conversation
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.
Copilot reviewed 19 out of 34 changed files in this pull request and generated no suggestions.
Files not reviewed (15)
- package.json: Language not supported
- scopes/component/stash/stash-data.ts: Evaluated as low risk
- scopes/harmony/aspect-loader/aspect-loader.main.runtime.ts: Evaluated as low risk
- scopes/component/stash/stash.main.runtime.ts: Evaluated as low risk
- scopes/dependencies/pnpm/pnpm.package-manager.ts: Evaluated as low risk
- scopes/component/isolator/isolator.main.runtime.ts: Evaluated as low risk
- scopes/component/merging/merging.main.runtime.ts: Evaluated as low risk
- scopes/dependencies/pnpm/lockfile-converter.ts: Evaluated as low risk
- scopes/component/snapping/tag-model-component.ts: Evaluated as low risk
- scopes/dependencies/dependency-resolver/index.ts: Evaluated as low risk
- scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts: Evaluated as low risk
- scopes/component/apply/apply.main.runtime.ts: Evaluated as low risk
- scopes/dependencies/dependencies/dependencies.main.runtime.ts: Evaluated as low risk
- scopes/component/forking/forking.main.runtime.ts: Evaluated as low risk
- scopes/dependencies/dependency-resolver/dependency-installer.ts: Evaluated as low risk
Comments skipped due to low confidence (1)
scopes/dependencies/dependency-resolver/package-manager.ts:220
- [nitpick] The variable name componentIdByPkgName is ambiguous. It should be renamed to componentIdByPackageName.
componentIdByPkgName: ComponentIdByPkgName;
this.logger.debug('installPackagesGracefully, start installing packages'); | ||
try { | ||
const installOpts = { | ||
dedupe: true, | ||
updateExisting: false, | ||
import: false, | ||
writeConfigFiles: !skipWriteConfigFiles, | ||
dependenciesGraph: await this.workspace.scope.getDependenciesGraphByComponentIds(componentIds), |
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.
Question: if I imported a few components (a,b), then I imported a few others (c,d), I will get here with the new ones (c,d) which means I'll build a graph based on c,d ids. which won't include a,b deps. isn't it will result in a wrong merged lock file?
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.
It will override the lockfile with dependencies of c and d. Dependencies of a and b will be resolved from scratch.
scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts
Outdated
Show resolved
Hide resolved
scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts
Outdated
Show resolved
Hide resolved
scopes/scope/update-dependencies/update-dependencies.main.runtime.ts
Outdated
Show resolved
Hide resolved
src/scope/models/version.ts
Outdated
|
||
static deserialize(data: string): DependenciesGraph | undefined { | ||
const parsed = JSON.parse(data); | ||
// If the schema version is not supported, then we just ignore the data |
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 believe that in most cases we will be able to support older data, maybe it's for future, but maybe add a todo here to have a better mechanism , like minimal supported schema version
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.
But we don't know what the future version of the data will look like. I understand that maybe when the new version will come out, the new bit CLI will be able to process the previous version too. But the old version of bit CLI can't process the future format.
Proposed Changes
Example of the dependencies graph object:
https://gist.github.com/zkochan/ff6c5ad4aea7d1aa1b27cfab746cea59
TODO:
don't write node_modules in e2e tests. We only need the lockfile.no way to skip install as next steps failin e2e tests skip preview in tagged componentspreviews are disabled in e2e tests by default