Graph: Remove Node interface #6530
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
|
joeyslater
approved these changes
Jun 29, 2021
added 2 commits
June 29, 2021 16:26
wbinnssmith
force-pushed
the
wbinnssmith/remove-node-interface
branch
from
2cb8176
to
f2a8a92
Compare
Benchmark ResultsKitchen Sink 🚨
Timings
Cold BundlesNo bundles found, this is probably a failed build... Cached BundlesNo bundles found, this is probably a failed build... React HackerNews ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. 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. |
AGawrys
approved these changes
Jun 30, 2021
lettertwo
added a commit
that referenced
this pull request
Jul 13, 2021
* v2: (34 commits) Wrap assets recursively when any incoming dependency is wrapped (#6572) Improvements for library targets (#6570) Diagnostic for undeclared external dependencies in library builds (#6564) More bugs (#6567) Don't require `url:` for image transformer (#6565) Remove 'Name already registered with serializer' error (#6566) Fix live bindings and `this` of external CommonJS modules (#6548) JS runtime improvements (#6531) Make sure the absolute path isn't contained in the cache (#5900) Adds '@parcel/diagnostic' to dependencies (#6563) Disable workers with string literals and improve diagnostics (#6536) Bug fixes (#6541) Don't attempt to resolve URLs starting with '#' (#6504) Correctly set worker's output format if not support by environment (#6534) Babel: Recognize peerDependencies in isJSX (#6497) fix setHeaders ordering on dev server (#6500) Graph: Remove Node interface (#6530) Fix TS build script for old Node versions (#6526) Improve library targets (#6517) Fix TypeScript and other sourcemaps by always creating an initial sourcemap (#6472) ...
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.
This removes the
Nodeinterface and its requirement for Graph values. This makesGraph<T>as generic asArray<T>.Rather than enforce an id property (which is vestigial and in our use usually reflects a content digest instead of the node's id in the graph) or a type property, users of Graph can use any shape for node values. This type can always be a disjoint union and the user can refine it in their code, but the Graph has never really been aware of the content of its values (nor should it be), so it no longer requires it.
I did not change any of the main AssetGraph, BundleGraph, or RequestGraph node value shapes, which still retain
typeandid.typeshould stay to form a disjoint union across the node values, butidshould probably be retired in the long term — it's really the content key.ContentGraph#addNodeByContentKeyno longer attempts to replace node values (it throws when a key already exists), and this behavior has been moved to where it has been expected.Test Plan:
yarn test. Adjusted unit tests and removed many of the shapes formerly needed to fulfill these interface requirements.