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

Decide on design for (and implement?) TrackedObject constructor #318

Open
Tracked by #316
chriskrycho opened this issue May 12, 2022 · 2 comments
Open
Tracked by #316

Decide on design for (and implement?) TrackedObject constructor #318

chriskrycho opened this issue May 12, 2022 · 2 comments
Labels

Comments

@chriskrycho
Copy link
Collaborator

chriskrycho commented May 12, 2022

Note: we can't actually start on this yet because Framework still needs to figure out what the semantics should be for it!

The simplest idea, “match the built-in constructors exactly” can have some weird effects:

let original = { foo: true };
let viaObject = new Object(original);
let viaTrackedObject = new TrackedObject(original);

original.foo = false;
console.log(viaObject.foo); // `false`
console.log(viaTrackedObject); // ???

viaTrackedObject.foo = true;
console.log(original.foo); // ???
console.log(viaObject.foo); // ???
  • We cannot actually intercept the set on original.foo, at least with the existing Proxy-based implementation, so if we return the (proxied) original we end up with cases where the object is sometimes mutated in a non-tracked fashion and sometimes mutated in a tracked fashion
  • If we directly walk and install setters for all (own?) properties instead, that probably works but needs to be very carefully documented and very carefully implemented

Additionally, we cannot ever make new TrackedObject() return the desired type from a TypeScript point-of-view.


Related: #73.

@chriskrycho chriskrycho changed the title <code class="notranslate">new TrackedObject()` should return the (proxy for the) original objectNote: we can't actually start on this yet because Framework still needs to figure out what the semantics should be for it. new TrackedObject() should return the (proxy for the) original object May 12, 2022
@chriskrycho chriskrycho changed the title new TrackedObject() should return the (proxy for the) original object Decide on design for TrackedObject constructor May 17, 2022
@chriskrycho chriskrycho changed the title Decide on design for TrackedObject constructor Decide on design for (and implement?) TrackedObject constructor May 17, 2022
@void-mAlex
Copy link

let me know if you see this as a standalone issue but I've found the current approach of cloning the object to use as a target of the Proxy breaks for private class properties

class MyObject {
  #original = null;
  //other props that I want to track
  get hasChanges(){
    return this.#original !== null && this.#original; //your changes checks here
  }
}
// elsewhere in code
const myObject = new MyObject();
const trackedObject  = new TrackedObject(myObject);
console.log(trackedObject.hasChanges); // error as private properties cannot be accessed

wondering if TrackedObject should instead be extended when trying to use it with private fields or if that is something we want to avoid altogether.

@ef4
Copy link
Contributor

ef4 commented Sep 15, 2023

wondering if TrackedObject should instead be extended when trying to use it with private fields

I checked and this works. And I think class MyThing extends TrackedObject {} is more coherent than new TrackedObject(new MyThing()), but I don't really love either.

I would be happier with a TrackedObject constructor that was limited to one-time field copying at construction, with no attempt at sharing a prototype or descriptors. Both because I don't think we can ever make private fields work, and because I'm dubious that this pattern is ever better than the alternatives.

If you don't know the shape of your data, it's incorrect to be mingling it with any prototype anyway. That's a prototype pollution bug waiting to happen. With unknown shapes you want the equivalent of Object.create(null).

If you do know the shape of your data, you can use @tracked normally.

If you don't control the implementation of the object and it has its own notion of reactivity that differs from @glimmer/tracking: that is the problem starbeam is working on and you aren't going to correctly solve it with just TrackedObject anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants