NullVoxPopuli / ember-resources

An implementation of Resources. Supports ember 3.28+
https://github.com/NullVoxPopuli/ember-resources/blob/main/docs/docs/README.md
MIT License
91 stars 37 forks source link

Require an owner to exist to be wired through to the resource. #1101

Closed NullVoxPopuli closed 6 months ago

NullVoxPopuli commented 6 months ago

Without this it's possible to break the public API of resource

for example:

const Clock = resource(({ owner }) => {
  owner.lookup(...)
});

would fail if the resource is constructed in a vanilla class that doesn't have an owner.

This likely will only affect tests.

stackblitz[bot] commented 6 months ago

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

github-actions[bot] commented 6 months ago

Estimated impact to a consuming app, depending on which bundle is imported

js min min + gzip min + brotli
/index.js 18.7 kB 4.41 kB 1.65 kB 1.45 kB
github-actions[bot] commented 6 months ago

Preview URLs

readme: https://e7d0fdcf.ember-resources.pages.dev api docs: https://e7d0fdcf.ember-resources.pages.dev/modules.html

vercel[bot] commented 6 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ember-resources ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 10, 2024 5:05pm
Techn1x commented 5 months ago

would fail if the resource is constructed in a vanilla class that doesn't have an owner.

Oof, I have exactly this usage, and am seeing the assertion failure (in dev) Cannot create resource without an owner

In particular, I have various trackedFunctions defined in vanilla classes like this

export class LessonPreview implements PreviewEngine {
  manifestBundle = trackedFunction(this, (): Promise<ActivityManifestBundle> => {
    return this.loader.fetchManifest(this.manifestPath)
  })
}

The class is instantiated in a component

class extends Component {
  constructor() {
    this.preview = new LessonPreview()
  }

  get previewBundle() {
    return this.preview.manifestBundle.value ? this.manifestBundle.value : {}
  }

  <template>{{this.previewBundle}}</template>
}

Am I using trackedFunction incorrectly here?

I know that from the sample code above it would seem I could just define the trackedFunction in the component, or even in a service, but the vanilla class provides a useful level of abstraction around configuration of each engine (I have many preview engines that need to be used across multiple components).

Techn1x commented 5 months ago

The approach I am taking is to use setOwner() to apply the same owner of the component, I think that's okay?

constructor() {
  this.preview = new LessonPreview()
  setOwner(this.preview, getOwner(this))
}
NullVoxPopuli commented 5 months ago

Yes, that's a good way to set the owner

bwbuchanan commented 5 months ago

I ran into this issue with ember-polaris-service and solved it with:

import type Owner from '@ember/owner';
import { setOwner } from '@ember/owner';
import Service, { service, type Scope } from 'ember-polaris-service';
import { use } from 'ember-resources';
import MyResource from './my-resource';

class MyService extends Service {
  constructor(scope: Scope) {
    super(scope);
    // ember-resources requires that we have an owner
    setOwner(this, scope as Owner);
  }

  myResource = use(this, MyResource);
}
NullVoxPopuli commented 5 months ago

why doesn't ember-polaris-service set its own owner? (like other services?) :thinking: