diff --git a/e2e/harmony/build-cmd.e2e.ts b/e2e/harmony/build-cmd.e2e.ts index f6804effef3a..5455e5ca3c98 100644 --- a/e2e/harmony/build-cmd.e2e.ts +++ b/e2e/harmony/build-cmd.e2e.ts @@ -213,6 +213,65 @@ describe('build command', function () { }); } ); + (supportNpmCiRegistryTesting ? describe : describe.skip)( + 'optimized capsule creation with a package dependency that points back into the capsule graph', + () => { + let npmCiRegistry: NpmCiRegistry; + before(async () => { + helper = new Helper({ scopesOptions: { remoteScopeWithDot: true } }); + helper.scopeHelper.setWorkspaceWithRemoteScope(); + npmCiRegistry = new NpmCiRegistry(helper); + await npmCiRegistry.init(); + npmCiRegistry.configureCiInPackageJsonHarmony(); + + // Build this graph, where comp3 is deliberately NOT a direct dev-dependency of the seeder: + // + // comp1 -(dev)-> comp2 -(prod)-> comp3 -(prod)-> comp4 + // | ^ + // `--------------------(dev)---------------------' + // + // comp4 is modified, so it must be a local capsule. Installing comp3 from the registry + // would make that package load its older comp4 dependency while comp1 loads the local + // comp4 capsule. The two SharedClass constructors then have different identities. + helper.fixtures.populateComponents(4); + helper.fs.outputFile('comp1/index.js', `module.exports = () => 'comp1';`); + const comp2PackageName = helper.general.getPackageNameByCompName('comp2', true); + const comp3PackageName = helper.general.getPackageNameByCompName('comp3', true); + const comp4PackageName = helper.general.getPackageNameByCompName('comp4', true); + helper.fs.outputFile('comp4/index.js', 'module.exports = class SharedClass {};'); + helper.fs.outputFile( + 'comp3/index.js', + `const SharedClass = require('${comp4PackageName}'); module.exports = /** @type {any} */ (new SharedClass());` + ); + helper.fs.outputFile('comp2/index.js', `module.exports = /** @type {any} */ (require('${comp3PackageName}'));`); + helper.fs.outputFile( + 'comp1/comp1.spec.js', + `const valueFromPackageClosure = require('${comp2PackageName}'); +const SharedClass = require('${comp4PackageName}'); + +describe('package and capsule generation identity', () => { + it('uses the capsule generation throughout the dependency closure', () => { + expect(valueFromPackageClosure).toBeInstanceOf(SharedClass); + }); +}); +` + ); + + helper.command.tagAllComponents(); + helper.command.export(); + helper.fs.appendFile('comp1/index.js', '\n// modification to comp1'); + helper.fs.appendFile('comp4/index.js', '\n// modification to comp4'); + helper.command.build('comp1'); + }); + after(() => { + npmCiRegistry.destroy(); + }); + it('keeps the transitive package dependency on the capsule side of the graph boundary', () => { + const comp3Capsule = helper.command.getCapsuleOfComponent(`${helper.scopes.remote}/comp3@0.0.1`); + expect(comp3Capsule).to.be.a.directory(); + }); + } + ); describe('optimized capsule creation for exported dependencies for self hosting', () => { before(async () => { helper = new Helper(); diff --git a/e2e/harmony/ci-commands.e2e.ts b/e2e/harmony/ci-commands.e2e.ts index a07f6ace4955..3936ad7c12f5 100644 --- a/e2e/harmony/ci-commands.e2e.ts +++ b/e2e/harmony/ci-commands.e2e.ts @@ -278,22 +278,35 @@ describe('ci commands', function () { describe('bit ci pr reuses the existing remote lane across subsequent PR commits', () => { let firstPrOutput: string; let secondPrOutput: string; + let comp3HeadAfterFirstRun: string; + let comp3HeadAfterSecondRun: string; before(() => { helper.scopeHelper.setWorkspaceWithRemoteScope(); setupGitRemote(); - setupComponentsAndInitialCommit(); + setupComponentsAndInitialCommit(3); + + const getComp3RemoteLaneHead = () => { + const lane = JSON.parse( + helper.command.runCmd(`bit lane show ${helper.scopes.remote}/feature-reuse-lane-test --remote --json`) + ); + const comp3 = lane.components.find((component: any) => component.id.endsWith('/comp3')); + if (!comp3) throw new Error(`comp3 was not found on the reused lane: ${JSON.stringify(lane.components)}`); + return comp3.head; + }; helper.command.runCmd('git checkout -b feature/reuse-lane-test'); - helper.fs.outputFile('comp1/comp1.js', 'console.log("first commit");'); - helper.command.runCmd('git add comp1/comp1.js'); + helper.fs.outputFile('comp3/comp3.js', 'console.log("first commit");'); + helper.command.runCmd('git add comp3/comp3.js'); helper.command.runCmd('git commit -m "feat: first commit"'); firstPrOutput = helper.command.runCmd('bit ci pr --keep-lane --message "first"'); + comp3HeadAfterFirstRun = getComp3RemoteLaneHead(); helper.fs.outputFile('comp2/comp2.js', 'console.log("second commit");'); helper.command.runCmd('git add comp2/comp2.js'); helper.command.runCmd('git commit -m "feat: second commit"'); secondPrOutput = helper.command.runCmd('bit ci pr --keep-lane --message "second"'); + comp3HeadAfterSecondRun = getComp3RemoteLaneHead(); }); it('should report that the lane was reused on the second run', () => { const cleanOutput = removeChalkCharacters(secondPrOutput) as string; @@ -310,6 +323,9 @@ describe('ci commands', function () { const matching = remoteLanes.lanes.filter((l: any) => l.name === 'feature-reuse-lane-test'); expect(matching).to.have.lengthOf(1); }); + it('should preserve the head of a component that was unchanged in the second run', () => { + expect(comp3HeadAfterSecondRun).to.equal(comp3HeadAfterFirstRun); + }); }); /** diff --git a/scopes/component/isolator/dependency-closed-package-set.spec.ts b/scopes/component/isolator/dependency-closed-package-set.spec.ts new file mode 100644 index 000000000000..b1c0d89203b6 --- /dev/null +++ b/scopes/component/isolator/dependency-closed-package-set.spec.ts @@ -0,0 +1,32 @@ +import { expect } from 'chai'; +import { ComponentID } from '@teambit/component-id'; +import type { ComponentIdGraph } from '@teambit/graph'; +import { enforceDependencyClosedPackageSet } from './dependency-closed-package-set'; + +describe('enforceDependencyClosedPackageSet', () => { + it('keeps closure propagation version-accurate when the graph contains multiple component versions', () => { + const consumerOfV1 = ComponentID.fromString('scope/consumer-v1@1.0.0'); + const consumerOfV2 = ComponentID.fromString('scope/consumer-v2@1.0.0'); + const dependencyV1 = ComponentID.fromString('scope/dependency@1.0.0'); + const dependencyV2 = ComponentID.fromString('scope/dependency@2.0.0'); + const nodes = new Map( + [consumerOfV1, consumerOfV2, dependencyV1, dependencyV2].map((id) => [id.toString(), { attr: id }]) + ); + const graph = { + edges: [ + { sourceId: consumerOfV1.toString(), targetId: dependencyV1.toString(), attr: 'prod' }, + { sourceId: consumerOfV2.toString(), targetId: dependencyV2.toString(), attr: 'prod' }, + ], + node: (id: string) => nodes.get(id), + } as unknown as ComponentIdGraph; + const capsuleIds = new Set([dependencyV2.toString()]); + const packageCandidateIds = new Set([consumerOfV1.toString(), consumerOfV2.toString(), dependencyV1.toString()]); + + enforceDependencyClosedPackageSet(graph, capsuleIds, packageCandidateIds); + + expect(capsuleIds).to.include(consumerOfV2.toString()); + expect(capsuleIds).to.not.include(consumerOfV1.toString()); + expect(packageCandidateIds).to.include(consumerOfV1.toString()); + expect(packageCandidateIds).to.include(dependencyV1.toString()); + }); +}); diff --git a/scopes/component/isolator/dependency-closed-package-set.ts b/scopes/component/isolator/dependency-closed-package-set.ts new file mode 100644 index 000000000000..47175bd7976d --- /dev/null +++ b/scopes/component/isolator/dependency-closed-package-set.ts @@ -0,0 +1,40 @@ +import type { ComponentIdGraph } from '@teambit/graph'; + +export type CapsulePromotion = { + dependentId: string; + dependencyId: string; +}; + +/** + * Mutate the capsule/package partition until every dependency of a package candidate is also a package candidate. + * IDs must include versions so different versions of the same component remain independent graph nodes. + */ +export function enforceDependencyClosedPackageSet( + graph: ComponentIdGraph, + capsuleIds: Set, + packageCandidateIds: Set +): CapsulePromotion[] { + const dependentsByDependencyId = new Map>(); + graph.edges.forEach((edge) => { + const sourceId = graph.node(edge.sourceId)?.attr?.toString(); + const targetId = graph.node(edge.targetId)?.attr?.toString(); + if (!sourceId || !targetId) return; + const dependents = dependentsByDependencyId.get(targetId) ?? new Set(); + dependents.add(sourceId); + dependentsByDependencyId.set(targetId, dependents); + }); + + const promotions: CapsulePromotion[] = []; + const capsuleQueue = [...capsuleIds]; + for (let queueIndex = 0; queueIndex < capsuleQueue.length; queueIndex += 1) { + const dependencyId = capsuleQueue[queueIndex]; + dependentsByDependencyId.get(dependencyId)?.forEach((dependentId) => { + if (!packageCandidateIds.delete(dependentId)) return; + capsuleIds.add(dependentId); + capsuleQueue.push(dependentId); + promotions.push({ dependentId, dependencyId }); + }); + } + + return promotions; +} diff --git a/scopes/component/isolator/isolator.main.runtime.ts b/scopes/component/isolator/isolator.main.runtime.ts index bd5e6bbf18cc..cf2cd75d66c0 100644 --- a/scopes/component/isolator/isolator.main.runtime.ts +++ b/scopes/component/isolator/isolator.main.runtime.ts @@ -12,7 +12,7 @@ import { ComponentMap, ComponentAspect } from '@teambit/component'; import type { ComponentMain, ComponentFactory, Component } from '@teambit/component'; import { getComponentPackageVersion, snapToSemver } from '@teambit/component-package-version'; import { createLinks } from '@teambit/dependencies.fs.linked-dependencies'; -import type { GraphMain } from '@teambit/graph'; +import type { ComponentIdGraph, GraphMain } from '@teambit/graph'; import { GraphAspect } from '@teambit/graph'; import type { SlotRegistry } from '@teambit/harmony'; import { Slot } from '@teambit/harmony'; @@ -68,6 +68,7 @@ import pMap from 'p-map'; import { Capsule } from './capsule'; import CapsuleList from './capsule-list'; import { CapsuleCache } from './capsule-cache'; +import { enforceDependencyClosedPackageSet } from './dependency-closed-package-set'; import type { CapsuleKind, PruneCapsulesOptions, PruneCapsulesReport } from './capsule-cache'; import { IsolatorAspect } from './isolator.aspect'; import { symlinkOnCapsuleRoot, symlinkDependenciesToCapsules } from './symlink-dependencies-to-capsules'; @@ -483,6 +484,7 @@ export class IsolatorMain { filteredComps, seeders, host, + successorsSubgraph, opts.originalSeeders ); this.logger.debug( @@ -1696,6 +1698,7 @@ export class IsolatorMain { components: Component[], seederIds: ComponentID[], host: ComponentFactory, + graph: ComponentIdGraph, originalSeeders?: ComponentID[] ): Promise { this.logger.debug(`filterUnmodifiedExportedDependencies: filtering ${components.length} components`); @@ -1704,7 +1707,8 @@ export class IsolatorMain { // @ts-ignore it's there, but we can't have the type of ScopeMain here to not create a circular dependency const remotes = await scope.getRemoteScopes(); - const filtered: Component[] = []; + const capsuleIds = new Set(); + const packageCandidateIds = new Set(); // Precompute version-normalized ID sets so membership is O(1) per component instead of a linear scan. // `toStringWithoutVersion()` is the string equivalent of `isEqual(id, { ignoreVersion: true })`. @@ -1719,7 +1723,7 @@ export class IsolatorMain { if (seederIdsNoVersion.has(componentIdNoVersion)) { // Always include seeders (modified components and their dependents) - filtered.push(component); + capsuleIds.add(componentIdStr); continue; } @@ -1729,7 +1733,7 @@ export class IsolatorMain { // then have to resolve it as an installed package instead of referencing the freshly-built capsule. // Keep it so the graph stays consistent across `bit build` and `bit tag` (both include it as a capsule). if (originalSeederIdsNoVersion?.has(componentIdNoVersion)) { - filtered.push(component); + capsuleIds.add(componentIdStr); continue; } // For dependencies, check if they are exported and unmodified @@ -1741,7 +1745,7 @@ export class IsolatorMain { const isModified = await component.isModified(); if (isModified) { // Always include modified components - filtered.push(component); + capsuleIds.add(componentIdStr); continue; } @@ -1763,12 +1767,36 @@ export class IsolatorMain { wasPublished; if (canBeInstalled) { - this.logger.debug(`[OPTIMIZATION] Excluding unmodified exported dependency: ${componentIdStr}`); + packageCandidateIds.add(componentIdStr); } else { - filtered.push(component); + capsuleIds.add(componentIdStr); } } + /** + * The package side of the optimized graph must be dependency-closed. If an installed package + * points back to a component kept as a capsule, Node can load both the package's recorded + * dependency version and the locally linked capsule version in one process. For stateful Bit + * modules this breaks class identity (`instanceof`), Harmony singletons, remotes, and caches. + * + * Start with components that must be capsules, then propagate backwards through every graph + * edge until no package candidate depends on a capsule. This handles runtime, dev, peer, and + * transitive crossings without disabling optimization for a dependency subtree that is wholly + * installable from the registry. + */ + const promotions = enforceDependencyClosedPackageSet(graph, capsuleIds, packageCandidateIds); + promotions.forEach(({ dependentId, dependencyId }) => + this.logger.debug( + `[OPTIMIZATION] Keeping ${dependentId} in capsule graph because it depends on capsule ${dependencyId}` + ) + ); + + const filtered = components.filter((component) => { + if (capsuleIds.has(component.id.toString())) return true; + this.logger.debug(`[OPTIMIZATION] Excluding unmodified exported dependency: ${component.id.toString()}`); + return false; + }); + this.logger.debug( `filterUnmodifiedExportedDependencies: kept ${filtered.length} out of ${components.length} components` );