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

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

Merged
merged 4 commits into from
Jan 10, 2024

Conversation

NullVoxPopuli
Copy link
Owner

@NullVoxPopuli NullVoxPopuli commented Jan 10, 2024

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.

@NullVoxPopuli NullVoxPopuli added bug Something isn't working breaking labels Jan 10, 2024
Copy link

stackblitz bot commented Jan 10, 2024

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

Copy link
Contributor

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

Copy link
Contributor

Copy link

vercel bot commented Jan 10, 2024

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

@NullVoxPopuli NullVoxPopuli merged commit 1eab27f into main Jan 10, 2024
26 checks passed
@NullVoxPopuli NullVoxPopuli deleted the require-an-owner branch January 10, 2024 17:11
@github-actions github-actions bot mentioned this pull request Jan 10, 2024
@Techn1x
Copy link

Techn1x commented Jan 23, 2024

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
Copy link

Techn1x commented Jan 23, 2024

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
Copy link
Owner Author

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

@bwbuchanan
Copy link

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
Copy link
Owner Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants