ember-resources icon indicating copy to clipboard operation
ember-resources copied to clipboard

feat: Make owner available to function-based resource

Open gossi opened this issue 3 years ago • 6 comments

Here is an idea, I mentioned earlier.

I thought I give it a spin to make the owner available to function based resources (class-based resources already have access to it).

This is incomplete. Was wondering if I'm on the right track here. Would add more tests then (at least a rendering test), what else?

gossi avatar Aug 22 '22 20:08 gossi

implementation looks good -- can you talk more about use case? want to make sure we don't add core functionality without proper thought / use-cases / etc. (seems useful! just want to make sure we defend it properly :muscle: )

NullVoxPopuli avatar Aug 22 '22 21:08 NullVoxPopuli

sure thing. Wanna combine ember-resources with ember-container into ember-abilities, which I wanna look like this:

const ability = (ability: (...args, contaiiner) => unknown) => resourceFactory((...args) => {
  // 

  return resource(({ on, owner }) => {
    // setup
    on.cleanup(() => {});
    
    const container = makeContainer(owner); // or maybe container directly ?

    // "the getter" of the resource
    return ability(...args, container);
  });
}));

which can be used as:

const canSeeBlogPost = ability((blogPost, { services }) => {
  const { user } = { services };
  const { currentUser } = { user }; // jaja, I know...

  return blogPost.author === currentUser;
});

which can be used as:

import { canSeeBlogPost } from '...';

<template>
  {{#if (canSeeBlogPost @post)}}
    show me the blog {{@post}}
  {{/if}}
</template>

so, need to check if rendering tests actually will update through this (or my whole journey is at risk 😬)

gossi avatar Aug 23 '22 15:08 gossi

that looks like a legit use case!

how do we feel about using getOwner / setOwner? (just being consistent with other embery things -- getOwner is also now properly typed, and using setOwner on the resource API allows us to not worry about the name of owner which feels like not-my-api-to-name, if that makes sense?

on the flip side, if we require getOwner that means we can't do the nice { on } destructuring... which I quite like. So now I'm back to providing owner maybe we support both

NullVoxPopuli avatar Aug 23 '22 21:08 NullVoxPopuli

Oh yes, I like that. Yeah, ember-container is basically the idea to have a destructuring owner. Would be an idea to have that as part of ember-resources and then be able to do that, but I doubt at this point. Currently, more of a community playground to start an issue at ember RFC repo after the first tries.

So I liked the idea with getOwner / setOwner, as it doesn't introduce another parameter - I updated the PR and included the rendering test. Now, resources themselves need to rely on getOwner() and thus are not yet pojo/standalone - but I can make ember-abilities bridge the gap here.

wdyt?

gossi avatar Aug 24 '22 20:08 gossi

wdyt?

looks nice!

but I think I'm learning towards supporting both destructured owner as well as getOwner. the maintenance aspect is near 0 on this library, so... why not support both? :sweat_smile:

NullVoxPopuli avatar Aug 24 '22 21:08 NullVoxPopuli

No, for two reasons:

  1. As much as I like destructuring the owner, this is a new concept and can and should mature, but I think this is the wrong place?
  2. There shall only be one API to get the owner in such a library. Only the one the library can provide (getOwner/setOwner). Destructuring can then still happen in userland 👍

gossi avatar Aug 25 '22 18:08 gossi

LGTM, thanks for the tests!

NullVoxPopuli avatar Sep 10 '22 18:09 NullVoxPopuli

:tada: This PR is included in version 5.4.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] avatar Sep 10 '22 19:09 github-actions[bot]