data-client icon indicating copy to clipboard operation
data-client copied to clipboard

RFC: Resource has an Entity (@rest-hooks/rest v6)

Open ntucker opened this issue 2 years ago • 2 comments

Motivation

Existing design

Resource is an Entity. Endpoints are defined as static members.

The motivation is for brevity: This allows one import to both define the expected type as well as access the endpoints to send as hook 'subjects'.

Problems

  1. Class static side is not well supported by TypeScript. This leads to the somewhat confusing but also limiting generic workaround.
  2. Inheritance does not work well for providing out-of-the-box endpoint definitions. Overrides are better
    • It's a struggle between general types that allow overrides or precise types that help developers.
    • Hacks like ‘SchemaDetail’ are an attempt around this but are confusing, expensive for typescript to compute and likely break in certain configurations.
  3. Union Resources are awkward to define as their expected schema ends up not being the Entity.
    • In general, custom schemas are often shared by multiple endpoints, making it desirable to not require them to be just an Entity
  4. Endpoints themselves don't maintain referential equality
    • This results in hacks in our hooks that violate expectations (ignoring referential changes to endpoints). There are legit reasons one might want to create a endpoint that changes and have that trigger fetches.

Solution: Resource Singletons

Resource Is-a-entity → Has-a-entity

Note: Entity being class is still fine as there is no type overrides expected.

Simplest case

class Todo extends Entity {
  readonly id: string = '';
  readonly content: string = '';

  pk() { return this.id }
}
const TodoResource = new Resource(Todo).extend(
  CRUD('http\\://test.com/todos' as const)
)
// this is where the schema 'has a'
assert(TodoResource.schema === Todo);
// these are now all very specific. will error if any fields are missing  and not exactly matching
const todo = useSuspense(TodoResource.detail, { id: '5' });
const todos = useSuspense(TodoResource.list);
controller.fetch(TodoResource.create, { content: 'ntucker' })
controller.fetch(TodoResource.update, { id: '5' }, { content: 'ntucker' })
controller.fetch(TodoResource.partialUpdate, { id: '5' }, { content: 'ntucker' })
controller.fetch(TodoResource.delete, { id: '5' })

Extra url parameter changes types

class User extends Entity {
  readonly id: number | undefined = undefined;
  readonly username: string = '';
  readonly email: string = '';
  readonly isAdmin: boolean = false;

  pk() {
    return this.id?.toString();
  }
}

const UserResource = new Resource(User).extend(
  CRUD('http\\://test.com/groups/:group/users' as const, 'id' as const),
).extend(resource => ({
  detail: resource.detail.extend({ schema: User2 }),
  another: resource.endpoint.extend({ schema: { data: resouce.schema, next: '' } }),
  ban: resource.endpoint.extend({ urlRoot: 'http\\://test.com/groups/:group/users/:id/ban' as const, method: 'post', sideEffect: true });
}));

const user = useSuspense(UserResource.detail, { group: 'five', id: '5' });
const users = useSuspense(UserResource.list, { group: 'five' });
controller.fetch(UserResource.create, { group: 'five' }, { username: 'ntucker' })
controller.fetch(UserResource.update, { group: 'five', id: '5' }, { username: 'ntucker' })
controller.fetch(UserResource.partialUpdate, { group: 'five', id: '5' }, { username: 'ntucker' })
controller.fetch(UserResource.delete, { group: 'five', id: '5' })

In the extend function, all ‘protected’ members are exposed (like fetch(), endpoint, etc). However, the only public members to UserResource are the endpoints (and the schema) - making typeahead exceptionally discoverable.

Entity Mixin

// entity as mixin
class Todo {
  readonly id: string = '';
  readonly content: string = '';
}
const TodoEntity = schema.Entity(Todo, todo => todo.id);
const TodoResource = new Resource(TodoEntity).extend(
  CRUD('http\\://test.com/todos' as const)
)

// need more entity lifecycles? just extend the mixin
class TodoEntity extends schema.Entity(Todo, todo => todo.id) {
  static useIncoming() {}
}

[Use an existing class for a resource · Issue #416 · coinbase/rest-hooks](https://github.com/coinbase/rest-hooks/issues/416)

Union

class Feed extends Entity {
  readonly id: string = '';
  readonly title: string = '';
  readonly type: 'link' | 'post' = 'post';
  pk() { return this.id }
}
class FeedLink extends Feed {
  readonly url: string = '';
  readonly type: 'link' = 'link';
}
class FeedPost extends Feed {
  readonly content: string = '';
  readonly type: 'post' = 'post';
}
const FeedUnion = schema.Union({post: FeedPost, link: FeedLink}, 'type' as const);
const FeedResource = new Resource(FeedUnion).extend(
  CRUD('http\\://test.com/feed' as const)
)

Tradeoffs

This does make it a bit more verbose

class Todo extends Entity {
  readonly id: string = '';
  readonly content: string = '';

  pk() { return this.id }
}
const TodoResource = new Resource(Todo).extend(
  CRUD('http\\://test.com/todos' as const)
)

vs

class TodoResource extends Resource{
  readonly id: string = '';
  readonly content: string = '';

  pk() { return this.id }

  static urlRoot = 'http\\://test.com/todos' as const
}

However, in practice this seems like a small sacrifice due to the common need for endpoint overrides, and how verbose that it. This has the added bonus of using the extra semantics to make the concepts more clear - Entity is simply a data description, and Resource is a set of operations (usually networking) to perform.

Alternatives considered

Rather than Resource.extend(), it might be more obvious to use an enhancer function (like a mixin). E.g., CRUD('url')(new Resource(User)). However, while this works fine for provided endpoint patterns like CRUD - it puts a burden on users doing custom overrides. Thus the 'extend()' method has been show to work well to automatically type, while providing override capabilities (see Endpoint).

Open question

  1. Change Resource.detail → Resource.get?
    1. Since this is an endpoint itself this makes more sense than before as calling get now actually does the ‘get’.
    2. Also Resource.list → Resource.getList?

ntucker avatar Aug 07 '22 22:08 ntucker

I like the CRUD function, but when you only want specific ones, maybe this?

/* This defines a create, read, and custom endpoint */
const UserResource = new Resource(User).extend({
/* partial CRUD function that returns a function with the arguments of CRUD */
...partial_CRUD('create', 'delete')('http\\://test.com/groups/:group/users' as const),
  custom: resource.endpoint.extend({ schema: { data: resouce.schema, next: '' } }),
})

sisuwk avatar Aug 12 '22 16:08 sisuwk

I would be happy to see new Resource(User).extend to allow multiple argument types. As long as a CRUD shape is returned, I can see having a method as an alternative argument when you need access to the super resource.

// 99% of the time just pass the crud shape
new Resource(User).extend({ ...CRUD_SHAPE })

// That time you need the `superResource`
new Resource(User).extend((superResource) => ({ ...CRUD_SHAPE }))

taystack avatar Aug 12 '22 17:08 taystack

New design iteration - all the members of Resource are just there to provide the createEndpoint. This is more intuitively and tersely modeled as extending the Endpoint class. Then Resources are simply collections of endpoints and nothing more. This creates a clear delineation of the holy trinity - Schema, Endpoint, Resource. These are all things that work together but completely disjoint.

While it is very clear how I want to model Endpoints. This makes Resource definitions a bit unclear. Here's three ideas I thought of specifically with the Github example use case as this required customizing each of the trinity for a base definition to use across all of github APIs.

Please compare each of the three or think of other ideas for Resource definitions

class IssueEndpoint extends GithubEndpoint {
  pollFrequency = 60000;
}

Resource/REST operations construction as member of endpoint

Theory here is the endpoint and operations are somewhat tightly coupled. It was awkward having to re-specify bindings between the two.

class GithubEndpoint extends RestEndpoint {
  async fetch(input: RequestInfo, init: RequestInit) {
    // we'll need to do the inverse operation when sending data back to the server
    if (init.body) {
      init.body = deeplyApplyKeyTransform(init.body, snakeCase);
    }
    // perform actual network request getting back json
    const jsonResponse = await super.fetch(input, init);
    // do the conversion!
    return deeplyApplyKeyTransform(jsonResponse, camelCase);
  }

  protected async processResponse(response: Response) {
    const results = await super.processResponse(response);
    if (
      (response.headers && response.headers.has('link')) ||
      Array.isArray(results)
    ) {
      return {
        link: response.headers.get('link'),
        results,
      };
    }
    return results;
  }

  Resource({ schema, urlRoot }) {
    const listSchema = { results: [schema], link: '' }
     return super.Resource({schema, urlRoot}).extend(resource => ({
      list: resource.list.extend({ schema: listSchema }),
      getNextPage: resource.list.extend({ schema: listSchema,  update: paginationUpdate(list, ({ page, ...rest } = {}) => [rest]) }),
     });
  }
}
const IssueResource = IssueEndpoint.Resource({
  schema: Issue,
  urlRoot: 'https\\://api.github.com/repos/:owner/:repo/issues/:number' as const,
}).extend(resource => ({
  current: new resource.Endpoint({
    urlRoot: 'https://api.github.com/user/',
    schema: resource.schema,
  })
})

Resource is just a POJO

class GithubEndpoint extends RestEndpoint {
  async fetch(input: RequestInfo, init: RequestInit) {
    // we'll need to do the inverse operation when sending data back to the server
    if (init.body) {
      init.body = deeplyApplyKeyTransform(init.body, snakeCase);
    }
    // perform actual network request getting back json
    const jsonResponse = await super.fetch(input, init);
    // do the conversion!
    return deeplyApplyKeyTransform(jsonResponse, camelCase);
  }

  protected async processResponse(response: Response) {
    const results = await super.processResponse(response);
    if (
      (response.headers && response.headers.has('link')) ||
      Array.isArray(results)
    ) {
      return {
        link: response.headers.get('link'),
        results,
      };
    }
    return results;
  }
}
function GithubResource({schema, urlRoot, Endpoint}) {
  const resource = Resource({schema, urlRoot, Endpoint});
  return {
    ...resource,
      list: resource.list.extend({ schema: listSchema }),
      getNextPage: resource.list.extend({ schema: listSchema,  update: paginationUpdate(list, ({ page, ...rest } = {}) => [rest]) }),
  }
}
const IssueResource = {
  ...GithubResource({
    schema: Issue,
    urlRoot: 'https\\://api.github.com/repos/:owner/:repo/issues/:number' as const,
    Endpoint: IssueEndpoint,
  }),
  current: new IssueEndpoint({
    urlRoot: 'https://api.github.com/user/',
    schema: Issue,
  })
}

Object that we must tell about our Endpoint

class GithubEndpoint extends RestEndpoint {
  async fetch(input: RequestInfo, init: RequestInit) {
    // we'll need to do the inverse operation when sending data back to the server
    if (init.body) {
      init.body = deeplyApplyKeyTransform(init.body, snakeCase);
    }
    // perform actual network request getting back json
    const jsonResponse = await super.fetch(input, init);
    // do the conversion!
    return deeplyApplyKeyTransform(jsonResponse, camelCase);
  }

  protected async processResponse(response: Response) {
    const results = await super.processResponse(response);
    if (
      (response.headers && response.headers.has('link')) ||
      Array.isArray(results)
    ) {
      return {
        link: response.headers.get('link'),
        results,
      };
    }
    return results;
  }
}

function  GithubResource({ schema, urlRoot }) {
    const listSchema = { results: [schema], link: '' }
     return Resource({schema, urlRoot}).extend(resource => ({
      list: resource.list.extend({ schema: listSchema }),
      getNextPage: resource.list.extend({ schema: listSchema,  update: paginationUpdate(list, ({ page, ...rest } = {}) => [rest]) }),
     });
  }
}
const IssueResource = GithubResource({
  schema: Issue,
  urlRoot: 'https\\://api.github.com/repos/:owner/:repo/issues/:number' as const,
  Endpoint: IssueEndpoint,
}).extend(resource => ({
  current: new resource.Endpoint({
    urlRoot: 'https://api.github.com/user/',
    schema: resource.schema,
  })
}))

Not possible but what looks cleanest

This fails because of inheritance requirements don't allow full overrides. I wish there was a way to tell TypeScript classes shouldn't be referred to as types directly so it could avoid those restrictions. (Essentially if you can never ask for a Resource, then there's no reason something that inherits from it has to be compatible.)

class GithubResource<U extends string, S extends Schema> extends Resource<U, S> {
  Endpoint = GithubEndpoint;
  list = this.list.extend({ schema: { results: [this.schema], link: '' }});
  getNextPage = this.list.extend({ schema: { results: [this.schema], link: '' },  update: paginationUpdate(list, ({ page, ...rest } = {}) => [rest]) });
}
class IssueResource<U extends string, S extends Schema> extends GithubResource<U, S> {
  Endpoint = IssueEndpoint;
  current = new this.Endpoint({ urlRoot: 'https://api.github.com/user/' as const, schema: this.schema });
}
export const Issues = new IssueResource({ schema: Issue, urlRoot: 'https://api.github.com/issues/:id' as const });

const issue = Issues.get({ id: '5' });

Inheritance without classes

function GithubResource<S extends Schema, U extends string, O>(schema: S, urlRoot: U, overrides: O) {
  return Resource(schema, urlRoot, resource => ({
      list: resource.list.extend({ schema: { results: [resource.schema], link: '' }}),
      getNextPage: resource.list.extend({ schema: { results: [resource.schema], link: '' },  update: paginationUpdate(list, ({ page, ...rest } = {}) => [rest]) }),
    ...overrides(resource),
  })
}
const Issues = GithubResource(
  Issue,
  'https://api.github.com/issues/:id' as const,
  resource => ({
    current: new Endpoint({ urlRoot: 'https://api.github.com/user/' as const, schema: this.schema }),
    ...overrides(resource)
  })
);

const issue = Issues.get({ id: '5' });

With object parameters

type Init = { Endpoint: RestEndpoint, schema: Schema, urlRoot: string, constructor: (resource: Resource) => Record<string, Endpoint> };

function GithubResource<P extends Init>({Endpoint, schema, urlRoot, constructor}: P) {
  return Resource({Endpoint, schema, urlRoot, constructor: resource => ({
       list: resource.list.extend({ schema: { results: [resource.schema], link: '' }}),
       getNextPage: resource.list.extend({ schema: { results: [resource.schema], link: '' },  update: paginationUpdate(list, ({ page, ...rest } = {}) => [rest]) }),
      ...constructor(resource),
    })
  })
}
const Issues = GithubResource({
  Endpoint: IssueEndpoint,
  schema: Issue,
  urlRoot: 'https://api.github.com/issues/:id' as const,
  constructor: resource => ({
    current: new resource.Endpoint({ urlRoot: 'https://api.github.com/user/' as const, schema: this.schema }),
    ...overrides(resource)
  })
});

const issue = Issues.get({ id: '5' });

Another iteration...combining function inheritance with tight coupling to Endpoint

class GithubEndpoint extends RestEndpoint {
  async fetch(input: RequestInfo, init: RequestInit) {
    // we'll need to do the inverse operation when sending data back to the server
    if (init.body) {
      init.body = deeplyApplyKeyTransform(init.body, snakeCase);
    }
    // perform actual network request getting back json
    const jsonResponse = await super.fetch(input, init);
    // do the conversion!
    return deeplyApplyKeyTransform(jsonResponse, camelCase);
  }

  protected async processResponse(response: Response) {
    const results = await super.processResponse(response);
    if (
      (response.headers && response.headers.has('link')) ||
      Array.isArray(results)
    ) {
      return {
        link: response.headers.get('link'),
        results,
      };
    }
    return results;
  }

  // this is probably a problem since it's a member of a class and our inherited types could use overrides which will make it not compatible
  Resource<P extends Init>({ schema, urlRoot, constructor }: P) {
    const listSchema = { results: [schema], link: '' }
     return super.Resource({schema, urlRoot, constructor: resource => ({
      list: resource.list.extend({ schema: listSchema }),
      getNextPage: resource.list.extend({ schema: listSchema,  update: paginationUpdate(list, ({ page, ...rest } = {}) => [rest]) }),
     })});
  }
}
type Init = { schema: Schema, urlRoot: string, constructor: (resource: Resource) => Record<string, Endpoint> };
const Issues = IssueEndpoint.Resource({
  schema: Issue,
  urlRoot: 'https://api.github.com/issues/:id' as const,
  constructor: resource => ({
    current: new resource.Endpoint({ urlRoot: 'https://api.github.com/user/' as const, schema: this.schema }),
    ...overrides(resource)
  })
});

const issue = Issues.get({ id: '5' });

ntucker avatar Aug 19 '22 16:08 ntucker