Skip to content

Conversation

@dobesv
Copy link
Contributor

@dobesv dobesv commented Mar 31, 2025

If you had a runnable task that depended on a noop task that in turn depended on a noop task, that depended on a runnable task, the dependency between the two runnable tasks would not be preserved. The logic that prunes out noop tasks in the dependency tree did not account for multiple levels of removal.

dobesv added 2 commits April 18, 2025 21:22
If you had a runnable task that depended on a noop task that in tern depended on a noop task, that depended on a runnable task, the dependency between the two runnable tasks would not be preserved.  The logic that prunes out noop tasks in the dependency tree did not account for multiple levels of removal.
for (const depId of node.dependencies) {
if (additionalDependencies.has(depId)) {
additionalDependencies.get(depId)!.forEach((subDepId) => newDependencies.add(subDepId));
const newDependencies = new Set<string>();
Copy link
Contributor Author

@dobesv dobesv Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: there is a major performance issue in this algorithm that I have fixed in my own fork. If you're interested in this pull request I can update it with the fix. For now I'm assuming you're not going to do anything with this pull request so there's no need to fix it.

The fix:

// Update dependencies of remaining nodes
  for (const node of nodeMap.values()) {
    const newDependencies = new Set<string>();
    const processedDeps = new Set<string>();

    // Inherit the dependencies of removed nodes we depended on
    const visitDependency = (depId: string) => {
      if (processedDeps.has(depId)) {
        return;
      }
      processedDeps.add(depId);
      if (nodeMap.has(depId)) {
        // It's still a valid node, keep in dependencies list
        newDependencies.add(depId);
      } else {
        // Node was removed, propagate dependencies from the removed node
        additionalDependencies.get(depId)?.forEach(visitDependency);
      }
    };
    node.dependencies.forEach(visitDependency);
    node.dependencies = Array.from(newDependencies);
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant