-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
feat(core): Augment data instead of copying it #5487
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach works, but it feels like we are re-inventing a number of things that we could get out of the box by using prototype chaining instead. As an example:
interface SomeObject {
[key:string]: any;
title: string;
version: string;
data: number;
}
const parent: SomeObject = {
title: "Parent",
version: "1",
data: 1,
}
Object.freeze(parent);
const child1: SomeObject = Object.create(parent);
child1.title = "Child1";
child1.version = "1.1";
parent.data = 5; // does not change!
const child2: SomeObject = Object.create(parent);
child2.title = "Child2";
child2.version = "1.2";
Object.freeze(child1);
child1.title = "Changed"; // does not change!
const grandChild: SomeObject = Object.create(child1);
grandChild.title = "GrandChild";
grandChild.version = "1.1.1";
function print(o: SomeObject) {
console.log(`${o.title} version: ${o.version} data: ${o.data} parent: ${o.__proto__.title}`);
}
print(parent); // 'Parent version: 1 data: 1 parent: undefined'
print(child1); // 'Child1 version: 1.1 data: 1 parent: Parent'
print(child2); // 'Child2 version: 1.2 data: 1 parent: Parent'
print(grandChild); // 'GrandChild version: 1.1.1 data: 1 parent: Child1'
Edit: I can see how this does not solve the recursive freezing and deleting of properties problem, so the Proxy solution is probably the right way 👍
Great PR! Please pay attention to the following items before merging: Files matching
Files matching
Make sure to check off this list before asking for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might be able to replace reflect-metadata calls with prototype chaining for possibly better performance. but that can be done in another PR.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #5487 +/- ##
==========================================
+ Coverage 13.05% 13.10% +0.04%
==========================================
Files 2462 2463 +1
Lines 112686 112761 +75
Branches 17498 17518 +20
==========================================
+ Hits 14715 14780 +65
- Misses 97471 97478 +7
- Partials 500 503 +3
... and 3 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Got released with |
Got released with |
Currently, Code & Function-Nodes copy a lot of data just to make sure that not by accident anything that should not change gets changed. This PR will enable the same but just in a cheaper way.