Correctly remove orphaned non-tree subgraphs

[mischnic]

↪️ Pull Request

removeNode(1) first removes 0-1, then 1-2 (because 2 is an orphan), then 2-3 (because 2 is an orphan after 0-1 was removed). The the right branch is visited and 2-3 is removed again which fails

I think it makes more sense to first remove the outgoing edges and then the incoming edges. This way, the subgraph is removed bottom up and a nodes becomes only orphaned once the last parent of that node is removed. So there will always be exactly one anchestor node that is responsible for removing it and no double deletion as before.

💻 Examples

The asset graph looked like this, changing some target option in package.json would remove the subgraph and run into a Edge from x to y not found error.

[parcel-benchmark]

Benchmark Results

Kitchen Sink 🚨

Timings

Description	Time	Difference
Cold	FAILED	-0.00ms
Cached	FAILED	-0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

React HackerNews ✅

Timings

Description	Time	Difference
Cold	9.78s	+431.00ms
Cached	475.00ms	-11.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

AtlasKit Editor 🚨

Timings

Description	Time	Difference
Cold	FAILED	-0.00ms
Cached	FAILED	-0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

Three.js ✅

Timings

Description	Time	Difference
Cold	6.55s	-84.00ms
Cached	300.00ms	+20.00ms ⚠️

Cold Bundles

No bundle changes detected.

Cached Bundles

Bundle	Size	Difference	Time	Difference
dist/Three.js	579.69kb	+0.00b	5.40s	+728.00ms ⚠️

Click here to view a detailed benchmark overview.

[mischnic]

Now this tests fails where b is removed and d is deleted twice

I wonder if we should just make the assertions in the methods less strict.

[mischnic]

@lettertwo @thebriando What do you think about removing some of these assertions in removeNode and removeEdge? I think finding an ordering/algorithm that doesn't have "double deletes" will be more effort (certainly to write it, and possibly also computationally) that just ignoring these additional deletions.

[lettertwo]

What do you think about removing some of these assertions in removeNode and removeEdge?

I was wondering this, too. I think we already do silent no-ops for the inverse operations (addNode and addEdge), so symmetrically, it would make sense to me. I don't currently know of a reason why would need to assert that a node or edge exists before removing it. I imagine any case that hinges on the success or failure of removal could be satisfied by returning a success boolean instead.

[thebriando]

What do you think about removing some of these assertions in removeNode and removeEdge?

Yep that makes sense to me as well.

[lettertwo]

I can't find a place where the result of removeEdge is handled. The only places I see where removeNode results are handled are in these two tests

[lettertwo]

I did find this old PR: #3562, but I'm not sure if the context around it is still relevant. /cc @wbinnssmith