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

Thoughts on this more ergonomic way to wire up the owner + destroyable association? #905

Open
NullVoxPopuli opened this issue Feb 28, 2023 · 2 comments

Comments

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Feb 28, 2023

Problem

there is a lot of boilerplate for creating vanilla classes that are properly wired up to the container.
For example:

import { cached } from '@glimmer/tracking';
import { getOwner, setOwner } from '@ember/owner';
import { associateDestroyableChild } from '@ember/destroyable';

class MyClass { /* ... */ }

export default class Demo extends Component {
  @cached 
  get myInstance() {
    let instance = new MyClass();
    
    associateDestroyableChild(this, instance);
    
    let owner = getOwner(this);

    if (owner) {
      setOwner(instance, owner);
    }

    return instance;
  }
}

Solution

this would be a new feature to ember in my ideal world, and not in a user-land package (even though it could live there)

Usage:

import { link } from '@ember/owner'; // proposed;

class MyClass { /* ... */ }

export default class Demo extends Component {
  @link myInstance = new MyClass();

  // alternatively, suggestion by Runspired
  @link(MyClass) declare myInstance: MyClass;
}
  • One import instead of 3
  • Two lines instead of 13
  • This is a common task -- and something that's easy to mess up as the order of params swaps between associateDestroyableChild and setOwner

Implementation:

function link(_prototype: object, key: string, descriptor?: Descriptor): void {
  if (!descriptor) return;

  assert(`@link can only be used with string-keys`, typeof key === 'string');


  let { initializer } = descriptor;

  assert(
    `@link may only be used on initialized properties. For example, ` +
      `\`@link foo = new MyClass();\``,
    initializer
  );


  let caches = new WeakMap<object, any>();

  // https://github.com/pzuraq/ember-could-get-used-to-this/blob/master/addon/index.js
  return {
    get(this: object) {
      let child = caches.get(this);

      if (!child) {
        child = initializer.call(this);

        associateDestroyableChild(this, child);

        let owner = getOwner(this);

        if (owner) {
          setOwner(child, owner);
        }

        caches.set(this, child);
        assert(`Failed to create cache for internal resource configuration object`, child);
      }

      return child;
    },
  } as unknown as void /* Thanks TS. */;
}

Disclaimer: I've given up with "properly typing" Stage 1 decorators, and instead assert my way in to the shapes I need.

Thoughts?


Some may wonder how you'd reactively manage args passed to MyClass, and the answer there is a Resource.
Reason being is that you can't use this pattern:

@cached 
get myInstance() {
  let instance = new MyClass(this.args.foo);
  
  link(this, instance); // overloaded
  
  return instance;
}

because you only get constructions of MyClass, and no destructions or updates when the args change.
This may be perfectly fine for some cases, but Resources are specifically for managing this destruction-or-update scenario where as the @cached getter is constructions only, until the parent class is destroyed (then all relevant destructors still remaining in memory would be called)
Sure, you could manage an "update" pattern via a local un-tracked variable such as:

_myInstance;

@cached 
get myInstance() {
  if (this._myInstance) {
     this._myInstance.update(this.args.foo);
     return this._myInstance;
  } 
  let instance = new MyClass(this.args.foo);
  
  link(this, instance); // overloaded
  
  this._myInstance = instance;
  return instance;
}

But wouldn't it feel much better to do this instead:

@use myInstance = MyClass.from(() => [this.args.foo]);
// or
myInstance = MyClass.from(this, () => [this.args.foo]);

ember-resources supports both of these APIs (with and without the decorator), and both are 100% TS safe / make sense to consumers. I need more feedback before one could be decided on -- and no matter what happens, there will be codemods <3

NullVoxPopuli added a commit to NullVoxPopuli/ember-resources that referenced this issue Mar 7, 2023
NullVoxPopuli added a commit to NullVoxPopuli/ember-resources that referenced this issue Mar 8, 2023
NullVoxPopuli added a commit to NullVoxPopuli/ember-resources that referenced this issue Mar 8, 2023
NullVoxPopuli added a commit to NullVoxPopuli/ember-resources that referenced this issue Mar 9, 2023
NullVoxPopuli added a commit to NullVoxPopuli/ember-resources that referenced this issue Mar 10, 2023
NullVoxPopuli added a commit to NullVoxPopuli/ember-resources that referenced this issue Mar 10, 2023
NullVoxPopuli added a commit to NullVoxPopuli/ember-resources that referenced this issue Mar 10, 2023
@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Mar 13, 2023

If anyone wants to this out, you can Mastodon | Twitter | Implementation PR and Changelog

you'll need an ember-resources@beta version to test it out as ember-resources v6 is still being tested / awaiting feedback on the breaking changes.

Something not mentioned anywhere is that if you want reactive arguments, you'd want this form:

export default class Demo extends Component {
  @link myInstance = new MyClass({
    foo: () => this.someTrackedProp,
  });
}

class MyClass {
  constructor(args) { this.args = args; }
  
  get reactiveGetterConsumingSomeTrackedProp() {
    return this.args.foo();
  }
}

@EWhite613
Copy link

So much cleaner and helps centralize some of the nastiness, where bugs could be fixed in one spot. I love this

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

No branches or pull requests

2 participants