Skip to content

RFC: useNewFetcher() #760

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

Closed
ntucker opened this issue Apr 17, 2021 · 5 comments
Closed

RFC: useNewFetcher() #760

ntucker opened this issue Apr 17, 2021 · 5 comments
Labels
enhancement New feature or request

Comments

@ntucker
Copy link
Collaborator

ntucker commented Apr 17, 2021

Would love feedback @dair-targ, @smith-it @jamakase @Secretmapper @Jsarihan @yunyu @praxxis @zacharyfmarion @kmartinezmedia

Motivation

Fixes #463, #458, #428, #316, #358

Lots of improvements can be made to useFetcher() design - both based on understanding of use cases, as well as evolution of interfaces like Endpoint.

Providing a new hook allows for incremental adoption of a complete redesign of many of the concepts incorporating 2 years of learnings and experience across many companies.

There are two major areas of change here

  • The hook and fetch itself
  • Programmable side-effects

References:

class User
class User extends Entity {
  readonly username: string = '';
  readonly firstName: string = '';
  readonly lastName: string = '';
  readonly isAdmin: boolean = false;
  pk() { retulrn this.username }

  get getterValue() { return this.username.toUpperCase(); }
  get fullName() { return `${this.firstName} ${this.lastName}`; }
}

Summary:

  • One hook, many endpoints
  • Completely flexible, variable arguments
  • Resolve/return value
  • Endpoint definition - new updater

Details

One hook, many endpoints

The rules of hooks are very restrictive, so the less hooks you have to call, the more flexible. This also benefits render performance. In many cases you might want to fetch many different endpoints. What's worse is if you don't know which endpoints you might want to fetch upfront. With old design you'd have to hook up every possible one. This really destroys fetch-as-render pattern, as you want to be able to prefetch based on possible routes.

Before

const createUser = useFetcher(User.create());
const refreshUsers = useFetcher(User.list());


return (
<form onSubmit={() => createUser({}, userPayload)}>
  <button onClick={() => refreshUsers({})}>Refresh list</button>
</form>
);

After

const fetch = useNewFetcher();

return (
<form onSubmit={() => fetch(User.create(), {}, userPayload)}>
  <button onClick={() => fetch(User.list(), {})}>Refresh list</button>
</form>
);

Completely flexible, variable arguments

The concept of params + body for arguments was introduced to try to provide the most flexible approach in a world where type enforcement wasn't that flexible. With TypeScript 4's variadic tuples, it's now possible to strongly type arbitrary arguments to a function in a generic way. Furthermore, stumbling upon package.json's typeVersions, rest hooks can now publish multiple type versions to be compatible with different versions of typescript. This allows us to eagerly adopt TypeScript 4 features, while providing a usable TypeScript 3 experience.

Some common annoyances with the current parameter limitations are single-variable arguments like detail endpoints with an id, as well as no-argument case like a list endpoint or create endpoint.

const fetch = useNewFetcher();

return <form onSubmit={() => fetch(User.create(), userPayload)}><button onClick={() => fetch(User.list())}>Refresh list</button></form>

We'll also eventually bring this to the 'read' hooks like so:

// notice username is just a string, rather than object
const user = useResource(User.detail(), username);
// here we don't need arguments
const posts = useResource(Post.list());
// but list() has it being optional, which means this also works:
const goodPosts = useResource(Post.list(), { good: true });
// postId is a number in this case
const thePost = useResource(Post.detail(), postId);

Resolve/return value

Before

Currently the promise from a useFetcher() resolves with exactly the data from the endpoint's fetch.

This is problematic for a few reasons:

  • These are typically typed with the Denormalize<Scheam> of the endpoint, which often deviates from the value by having classes instead of pojos.
  • In some cases having access to the classes itself is useful - for instance using class methods, getters, etc. Or even using the value to compare with what comes out of useCache/useResource

Currently the resolution of the promise happens upon commit of the updates to the cache state. This guarantees any updates from the fetch exist in the current react render/DOM before processing the next step.

After

Idea 1) Just change resolution to denormalized value
const fetch = useNewFetcher();

const onClick = async () => {
  const userEntity  = await fetch(createUser, userPayload);
  assert(userEntity instanceof User);
  assert(data.getterValue !== undefined);
  return userEntity;
}

Pros:

  • simple revision, easy to use

Cons:

  • normalize+demoralize computation must be called for every fetch.
    • in practice, I've found that the majority of cases, the actual resolved value is not needed so this is some amount of extra CPU work that is wasted
    • We could provide two hooks - one that does this and one that doesn't. However, this introduces a larger API surface area.
Idea 2) Two stage promises

This sort of matches the fetch pattern - where the headers like status are available immediately, but json() returns another promise.

This also enables distinguishing the stages of a fetch - the resolution of fetch itself, and the commit of the updates to the DOM and react tree.

const fetch = useNewFetcher();

const onClick = async () => {
  const { data, commit } = await fetch(createUser, userPayload);
  assert(isPojo(data));
  assert(data.getterValue === undefined);
  // no DOM updates - react is still rendering previous value
  const userEntity = await commit();
  // now any DOM operations based on this resolution are complete
  assert(userEntity instanceof User);
  assert(data.getterValue !== undefined);
  return userEntity;
}

Pros

  • we can also distinguish between different phases of resolution
    • tho it's questionable the utility of doing anything before commit in what will always be react-land code. this might introduce more bugs
  • Only do computation when needed

Cons

  • common case slightly more complicated as you have to index into .data.
    • This is another thing to know, however it is quite discoverable in typescript so not super terrible.

Endpoint definition - new updater

By normalizing Entities, Rest Hooks guarantees data integrity and consistency even down to the referential equality level. However, there are still some cases where side effects result in changes to the actual results themselves. The most common reason for this is creation of new entities. While 'creation' is almost universally the cause for this (as deletion is handled more simply by delete schemas), the structure of data and where created elements go is not universal.

Before

Previously this was enabled by an optional third argument to the fetch UpdateParams enabling programmatic changes that are also strictly type enforced to ensure the data integrity of the Rest Hooks store.

const createArticle = useFetcher(ArticleResource.create());

createArticle({}, { id: 1 }, [
  [
    ArticleResource.list(),
    {},
    (newArticleID: string, articleIDs: string[] | undefined) => [
      ...(articleIDs || []),
      newArticleID,
    ],
  ],
]);

While simple, this design had several shortcomings

  • Only operates on the normalized results, often arrays of strings
    • This is non-intuitive as this doesn't relate directly to the data's form and requires understanding of internals
    • Code is confusing with two ordered args and necessary default handling
    • Lack of access to entities means sorting is not possible
    • Can only update top level results, which means lists nested inside entities cannot be updated
  • Is provided as an argument to the fetch rather than endpoint
    • Makes variable arguments impossible, and hard to reason about
    • Makes pattern reuse still require explicit wiring
    • Was thought to be more flexible than in 'fetchshape', as it has access to local variables in its event handler. However, Endpoints's can easily use .extend() to contextually override so this feature is moot.
    • Encourages antipatterns like writing hooks for specific endpoints

After

  • Operate on the actual denormalized form - that is the same shape that is consumsed with a useResource()
  • Move to Endpoint
  • Take the denormalized response as arg to first function
  • builder pattern to make updater definition easy
    • typeahead
    • strong type enforcement
    • much more readable than a size 3 tuple

Simplest case:

type UserList = Denormalized<typeof userList['schema']>

const createUser = new Endpoint(postToUserFunction, {
  schema: User,
  update: (newUser: Denormalize<S>)  => [ userList.bind().updater((users: UserList = []) => [newUser, ...users]) ],
});

More updates:

Component.tsx
const allusers = useResource(userList);
const adminUsers = useResource(userList, { admin: true });
const sortedUsers = useResource(userList, { sortBy: 'createdAt' });

The endpoint below ensures the new user shows up immediately in the usages above.

userEndpoint.ts
const createUser = new Endpoint(postToUserFunction, {
  schema: User,
  update: (newUser: Denormalize<S>)  => {
    const updates = [
      userList.bind().updater((users = []) => [newUser, ...users]),
      userList.bind({ sortBy: 'createdAt' }).updater((users = [], { sortBy }) => {
        const ret = [createdUser, ...users];
        ret.sortBy(sortBy);
        return ret;
      },
    ];
    if (newUser.isAdmin) {
      updates.push(userList.bind({ admin: true }).updater((users = []) => [newUser, ...users]));
    }
    return updates;
  },
});

Extracting patterns

In case more than one other endpoint might result in updating our list endpoint, we can centralize the logic of how that should work in our updated endpoint.

const userList = new Endpoint(getUsers, {
  schema: User[],
  addUserUpdater: (this: Endpoint, newUser: User) => this.updater((users = []) => [newUser, ...users]),
});
const createUser = new Endpoint(postToUserFunction, {
  schema: User,
  update: (newUser: Denormalize<S>) => [
    userList.bind({ admin: true }).addUserUpate(newUser),
    userList.bind({ }).addUserUpate(newUser),
  ]
});
Alternate Ideas - The programmatic approach

The no guarantees

const createUser = new Endpoint(postToUserFunction, {
  schema: User,
  update: (newUser: Denormalize<S>, state: State<unknown>) => {
    return {
      ...state,
      results: {
        ...state.results,
       [userList.key({ admin: true })]: [newUser.pk(), ...state.results[userList.key({ admin: true })]],
       [userList.key({ })]: [newUser.pk(), ...state.results[userList.key({ })]],
     }
    }
  }
}

Store Adapter

const createUser = new Endpoint(postToUserFunction, {
  schema: User,
  update: (newUser: Denormalize<S>, store: Store) => {
    const prependUser = (users = []) => [newUser, ...users]
    if (newUser.isAdmin) {
      store = store.set(
         userList.bind({admin: true}),
         prependUser 
      );
    }
    store = store.set(
       userList.bind({}),
       prependUser
    );
    return store;
  }
}
const createUser = new Endpoint(postToUserFunction, {
  schema: User,
  update: (newUser: Denormalize<S>, store: Store) => {
    // this actually goes through every current result based on this endpoint
    // however it does not update extremely stale results for performance reasons
    store.get(userList).mapItems((key: string, users: User[]) => {
      if (!key.includes('admin') || newUser.isAdmin) {
        return [newUser, ...users];
      }
    });
  }
}

Another idea is to make an updater callback with identical API to manager middleware. We would probably want to minimize chaining actions, so some way of consolidating into one action would be preferable. Adding an adapter to raw state might be good for Manager's as well, so designing this interface could be beneficial to optionally improving middleware interfaces.

@praxxis
Copy link

praxxis commented Apr 21, 2021

@ntucker at first glance this seems neat but to be honest I think I'm lacking some detail around Endpoint's. I just saw the .bind PR which helped. Would you be able to run through the first example in detail maybe? For example, the admin updater passes admin: true to bind, which presumably is the query param sent to the server, but then also checks createdUser.isAdmin, which seems redundant .

@ntucker
Copy link
Collaborator Author

ntucker commented Apr 22, 2021

@praxxis Thanks for the response. Updated the description pretty heavily - both based on your questions and some evolution of my thoughts. Also reorganized the post to clearly delineate each change in its own section.

@sweetro
Copy link
Contributor

sweetro commented Apr 23, 2021

Adding an instance I encountered where a two stage promise would be helpful. This is within a modal that deletes a resource and closes on delete success.

const data = useResource(SomeResource.detail() { id })
const doDelete = useFetcher(SomeResource.delete()) 
// later on
onClick = async () => { 
  await doDelete({ id })
  closeModal()
}

In this case, the component is still mounted when the delete completes, so useResource refetches, deterministically triggering a 400. A two stage promise should allow the component to unmount when the request succeeds but before the change is committed to the resthooks cache.

@ntucker
Copy link
Collaborator Author

ntucker commented May 17, 2021

@sweetro This is very unexpected. It should be resolving that promise before the state is updated - unless you're using optimistic updates? If optimistic updates are used you'll want to just rearrange the lines:

const data = useResource(SomeResource.detail() { id })
const doDelete = useFetcher(SomeResource.delete()) 
// later on
onClick = async () => { 
  closeModal()
  await doDelete({ id })
}

@ntucker
Copy link
Collaborator Author

ntucker commented Sep 14, 2021

Resolved with #1239

@ntucker ntucker closed this as completed Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants