Support multiple edge types in Graph.hasEdge
#8550
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark ResultsKitchen Sink ✅
Timings
Cold Bundles
Cached BundlesNo bundle changes detected. React HackerNews 🚨
Timings
Cold BundlesNo bundles found, this is probably a failed build... Cached BundlesNo bundles found, this is probably a failed build... AtlasKit Editor 🚨
Timings
Cold BundlesNo bundles found, this is probably a failed build... Cached BundlesNo bundles found, this is probably a failed build... Three.js ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. |
thebriando
reviewed
Oct 17, 2022
mattcompiles
commented
Oct 18, 2022
There was a problem hiding this comment.
@thebriando I made the changes we spoke about. Had a bit of trouble with the types, I think a lot of the serialize/deserialze types are not quite right which I ended up changing in some cases.
Let me know what you think.
mattcompiles
changed the title
Graph.hasEdge
thebriando
approved these changes
Oct 19, 2022
AGawrys
approved these changes
Oct 19, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
↪️ Pull Request
This PR adds some desired functionality discovered when creating #8453. @AGawrys
Currently,
Graph.hasEdgeonly accepts a single edge type to validate against. This PR adds support for:TheALL_EDGE_TYPESconstant, allowing edge checks to be type agnostic.As the graph isn't currently aware of all possible edge types without doing lookups, we (@alshdavid and I) have added a newedgeTypesproperty to theAdjacencyListwhich is appended insideaddEdge. To keep things simple, we never remove edge types. This shouldn't cause any issues, but does mean it's possible to search for edge types that no longer exist in the graph and edge types are never cleaned up once they have existed.It's possible we'll decide that supportingALL_EDGE_TYPESisn't worth tracking the extra piece of state. If that's the case it should be easy to remove that code and keep just the passed array of edge types param.After a discussion with @AGawrys we've decided simple array support is enough and
ALL_EDGE_TYPESisn't worth the complexity.💻 Examples
✔️ PR Todo