Skip to content
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(core): ensure to touch all nodes in package-graph, fix issue found by Jest team #301

Merged
merged 1 commit into from
Aug 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"dist-list-cmd": "node ./packages/cli/dist/cli.js list --all",
"dist-roll-version-dry-run": "node ./packages/cli/dist/cli.js version --git-dry-run",
"dist-roll-publish-dry-run": "node ./packages/cli/dist/cli.js publish from-package --git-dry-run",
"dist-roll-publish-alpha-dry-run": "node ./packages/cli/dist/cli.js publish --git-dry-run --exact --include-merged-tags --preid alpha --dist-tag next prerelease",
"dist-exec-win": "node ./packages/cli/dist/cli.js exec --scope {@lerna-lite/cli,@lerna-lite/core} -- echo hello from package: %LERNA_PACKAGE_NAME%",
"dist-exec-unix": "node ./packages/cli/dist/cli.js exec -- echo hello from package: ${LERNA_PACKAGE_NAME}",
"dist-pack-tarball": "node ./packages/cli/dist/cli.js run pack-tarball",
Expand Down
27 changes: 13 additions & 14 deletions packages/core/src/package-graph/package-graph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ import { NpaResolveResult } from '../models';
*
* @extends {Map<string, PackageGraphNode>}
*/
export class PackageGraph extends Map {
export class PackageGraph extends Map<string, PackageGraphNode> {
/**
* @param {import("@lerna/package").Package[]} packages An array of Packages to build the graph out of.
* @param {Package[]} packages - An array of Packages to build the graph out of.
* @param {'allDependencies'|'dependencies'} [graphType]
* Pass "dependencies" to create a graph of only dependencies,
* excluding the devDependencies that would normally be included.
Expand All @@ -28,7 +28,6 @@ export class PackageGraph extends Map {
localDependencies = 'force'; // eslint-disable-line
}

// @ts-ignore
super(packages.map((pkg: Package) => [pkg?.name ?? '', new PackageGraphNode(pkg)]));

if (packages.length !== this.size) {
Expand Down Expand Up @@ -127,7 +126,7 @@ export class PackageGraph extends Map {
* they depend on. i.e if packageA depended on packageB `graph.addDependencies([packageA])`
* would return [packageA, packageB].
*
* @param {import("@lerna/package").Package[]} filteredPackages The packages to include dependencies for.
* @param {Package[]} filteredPackages - The packages to include dependencies for.
*/
addDependencies(filteredPackages: Package[]) {
return this.extendList(filteredPackages, 'localDependencies');
Expand All @@ -138,7 +137,7 @@ export class PackageGraph extends Map {
* that depend on them. i.e if packageC depended on packageD `graph.addDependents([packageD])`
* would return [packageD, packageC].
*
* @param {import("@lerna/package").Package[]} filteredPackages The packages to include dependents for.
* @param {Package[]} filteredPackages - The packages to include dependents for.
*/
addDependents(filteredPackages: Package[]) {
return this.extendList(filteredPackages, 'localDependents');
Expand All @@ -149,16 +148,15 @@ export class PackageGraph extends Map {
* `PackageGraphNode` property that is a collection of `PackageGraphNode`s.
* Returns input packages with any additional packages found by traversing `nodeProp`.
*
* @param {import("@lerna/package").Package[]} packageList The list of packages to extend
* @param {'localDependencies'|'localDependents'} nodeProp The property on `PackageGraphNode` used to traverse
* @param {Package[]} packageList - The list of packages to extend
* @param {'localDependencies'|'localDependents'} nodeProp - The property on `PackageGraphNode` used to traverse
*/
extendList(packageList: Package[], nodeProp: 'localDependencies' | 'localDependents') {
// the current list of packages we are expanding using breadth-first-search
const search = new Set<PackageGraphNode>(packageList.map(({ name }) => this.get(name)));
const search = new Set<PackageGraphNode>(packageList.map(({ name }) => this.get(name) as PackageGraphNode));

// an intermediate list of matched PackageGraphNodes
/** @type {PackageGraphNode[]} */
const result: PackageGraphNode[] = [];
const result: Array<PackageGraphNode> = [];

search.forEach((currentNode) => {
// anything searched for is always a result
Expand All @@ -167,7 +165,7 @@ export class PackageGraph extends Map {
currentNode[nodeProp].forEach((meta, depName) => {
const depNode = this.get(depName);

if (depNode !== currentNode && !search.has(depNode)) {
if (depNode && depNode !== currentNode && !search.has(depNode)) {
search.add(depNode);
}
});
Expand Down Expand Up @@ -245,7 +243,7 @@ export class PackageGraph extends Map {
const cyclePaths: string[] = [];
const nodeToCycle = new Map<PackageGraphNode, CyclicPackageGraphNode>();
const cycles = new Set<CyclicPackageGraphNode>();
const alreadyVisited = new Set<PackageGraphNode>();
const alreadyVisited = new Set<string>();
const walkStack: Array<PackageGraphNode | CyclicPackageGraphNode> = [];

function visits(baseNode, dependentNode) {
Expand All @@ -259,10 +257,11 @@ export class PackageGraph extends Map {
}

// Otherwise the same node is checked multiple times which is very wasteful in a large repository
if (alreadyVisited.has(topLevelDependent)) {
const identifier = `${baseNode.name}:${topLevelDependent.name}`;
if (alreadyVisited.has(identifier)) {
return;
}
alreadyVisited.add(topLevelDependent);
alreadyVisited.add(identifier);

if (topLevelDependent === baseNode || (topLevelDependent.isCycle && topLevelDependent.has(baseNode.name))) {
const cycle: any = new CyclicPackageGraphNode();
Expand Down
4 changes: 2 additions & 2 deletions packages/diff/src/diff-command.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Command, CommandType, DiffCommandOption, Package, spawn, ValidationError } from '@lerna-lite/core';
import { Command, CommandType, DiffCommandOption, PackageGraphNode, spawn, ValidationError } from '@lerna-lite/core';

import { getLastCommit } from './lib/get-last-commit';
import { hasCommit } from './lib/has-commit';
Expand All @@ -17,7 +17,7 @@ export class DiffCommand extends Command<DiffCommandOption> {
}

initialize() {
let targetPackage: Package | undefined = undefined;
let targetPackage: PackageGraphNode | undefined = undefined;
const packageName = this.options.pkgName;

if (packageName) {
Expand Down
6 changes: 3 additions & 3 deletions packages/publish/src/publish-command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ export class PublishCommand extends Command<PublishCommandOption> {

for (const [depName, resolved] of node.localDependencies) {
// other canary versions need to be updated, non-canary is a no-op
const depVersion = this.updatesVersions?.get(depName) || this.packageGraph?.get(depName).pkg.version;
const depVersion = this.updatesVersions?.get(depName) || this.packageGraph?.get(depName)!.pkg.version;

// it no longer matters if we mutate the shared Package instance
node.pkg.updateLocalDependency(
Expand All @@ -588,7 +588,7 @@ export class PublishCommand extends Command<PublishCommandOption> {
return pMap(updatesWithLocalLinks, (node: PackageGraphNode) => {
for (const [depName, resolved] of node.localDependencies) {
// regardless of where the version comes from, we can't publish 'file:../sibling-pkg' specs
const depVersion = this.updatesVersions?.get(depName) || this.packageGraph?.get(depName).pkg.version;
const depVersion = this.updatesVersions?.get(depName) || this.packageGraph?.get(depName)!.pkg.version;

// it no longer matters if we mutate the shared Package instance
node.pkg.updateLocalDependency(
Expand Down Expand Up @@ -616,7 +616,7 @@ export class PublishCommand extends Command<PublishCommandOption> {

// 1. update & bump version of local dependencies
for (const [depName, resolved] of node.localDependencies) {
const depVersion = this.updatesVersions?.get(depName) || this.packageGraph?.get(depName).pkg.version;
const depVersion = this.updatesVersions?.get(depName) || this.packageGraph?.get(depName)!.pkg.version;

// it no longer matters if we mutate the shared Package instance
node.pkg.updateLocalDependency(
Expand Down
4 changes: 2 additions & 2 deletions packages/version/src/version-command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,7 @@ export class VersionCommand extends Command<VersionCommandOption> {
let hasBreakingChange = false;

for (const [name, bump] of versions) {
hasBreakingChange = hasBreakingChange || isBreakingChange(this.packageGraph?.get(name).version, bump);
hasBreakingChange = hasBreakingChange || isBreakingChange(this.packageGraph?.get(name)!.version, bump);
}

if (hasBreakingChange) {
Expand Down Expand Up @@ -610,7 +610,7 @@ export class VersionCommand extends Command<VersionCommandOption> {
pkg.set('version', this.updatesVersions?.get(pkg?.name ?? ''));

// update pkg dependencies
for (const [depName, resolved] of this.packageGraph?.get(pkg.name).localDependencies) {
for (const [depName, resolved] of this.packageGraph?.get(pkg.name)!.localDependencies) {
const depVersion = this.updatesVersions?.get(depName);

if (depVersion && resolved.type !== 'directory') {
Expand Down