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

Ensure edge exists before removal in Graph.removeEdge #8554

Merged
merged 6 commits into from Oct 24, 2022

Conversation

mattcompiles
Copy link
Contributor

↪️ Pull Request

Prior to PR #7927, removeEdge validated that an edge existed before attempting to remove it. The assertions were removed as the recursive nature of removeEdge could lead to edges being deleted twice.

While ignoring double removals makes sense when orphaned nodes/edges are being cleaned up, it's not ideal when calling into removeEdge externally. It can lead to a confusing developer experience where removeEdge is called incorrectly on edges that don't exist but the Graph just silently ignores the request.

This PR splits removeEdge into separate private and public APIs.

  • _removeEdge - only used internally to the Graph, ignores edge delete requests that don't exist
  • removeEdge - validates the edge exists and then calls _removeEdge, throws if the edge doesn't exist

One drawback of this approach is that calls to removeEdge validate that the edge exists twice.

💻 Examples

let graph = new Graph();
let nodeA = graph.addNode('a');
let nodeB = graph.addNode('b');

assert.throws(() => {
  graph.removeEdge(nodeA, nodeB);
}, /Edge from 0 to 1 not found!/);

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@parcel-benchmark
Copy link

parcel-benchmark commented Oct 18, 2022

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 1.36s +7.00ms
Cached 298.00ms -32.00ms 🚀

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

React HackerNews 🚨

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...

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.33s -78.00ms
Cached 290.00ms +13.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

@mattcompiles mattcompiles merged commit 8b8e650 into v2 Oct 24, 2022
@mattcompiles mattcompiles deleted the mattcompiles/validate-remove-edge branch October 24, 2022 22:22
@yrral86
Copy link

yrral86 commented Nov 15, 2022

After upgrading to 2.8.0, I am seeing

🚨 Build failed.

@parcel/bundler-default: Edge from 2050 to 2051 not found!

  Error: Edge from 2050 to 2051 not found!
      at ContentGraph.removeEdge (/redacted/node_modules/@parcel/graph/lib/Graph.js:156:13)
      at MutableBundleGraph.createBundleGroup (/redacted/node_modules/@parcel/core/lib/public/MutableBundleGraph.js:134:24)
      at decorateLegacyGraph (/redacted/node_modules/@parcel/bundler-default/lib/DefaultBundler.js:159:35)
      at Object.bundle (/redacted/node_modules/@parcel/bundler-default/lib/DefaultBundler.js:126:7)
      at BundlerRunner.bundle (/redacted/node_modules/@parcel/core/lib/requests/BundleGraphRequest.js:288:23)
      at async Object.run (/redacted/node_modules/@parcel/core/lib/requests/BundleGraphRequest.js:156:17)
      at async RequestTracker.runRequest (/redacted/node_modules/@parcel/core/lib/RequestTracker.js:756:20)
      at async Object.run (/redacted/node_modules/@parcel/core/lib/requests/ParcelBuildRequest.js:56:7)
      at async RequestTracker.runRequest (/redacted/node_modules/@parcel/core/lib/RequestTracker.js:756:20)
      at async Parcel._build (/redacted/node_modules/@parcel/core/lib/Parcel.js:400:11)

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.

None yet

5 participants