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

Adding a class-based layer above, any obvious pitfalls? #87

Closed
natew opened this issue May 17, 2020 · 3 comments
Closed

Adding a class-based layer above, any obvious pitfalls? #87

natew opened this issue May 17, 2020 · 3 comments

Comments

@natew
Copy link

natew commented May 17, 2020

I can't help but think the functional approach to recoil really doesn't work well with JS.

This is the example from the docs:

const todoListFilterState = atom({
  key: 'todoListFilterState',
  default: 'Show All',
});

const filteredTodoListState = selector({
  key: 'filteredTodoListState',
  get: ({get}) => {
    const filter = get(todoListFilterState);
    const list = get(todoListState);

    switch (filter) {
      case 'Show Completed':
        return list.filter((item) => item.isComplete);
      case 'Show Uncompleted':
        return list.filter((item) => !item.isComplete);
      default:
        return list;
    }
  },
});

Here's it re-written as a class:

class TodoListStore {
  list = []
  filter = ''

  get filtered() {
    switch (this.filter) {
      case 'Show Completed':
        return this.list.filter((item) => item.isComplete);
      case 'Show Uncompleted':
        return this.list.filter((item) => !item.isComplete);
      default:
        return this.list;
    }
  }
}

I think you could make a translation layer above recoil to make the above work in just a few minutes of work really, though perhaps there are some edge cases. It has the following advantages:

  • It's more direct and lets you "think in the language", ie, I get to reason about how to change state like I do normally in programming. I just set things like normal, no API to learn, no get/set.
  • It's far less verbose
  • No need to learn library-only concepts (atom, selector)
  • It can compile to smaller bundles by mangling class property names

One interesting piece is it forces you to group state logically. To me this seems like yet another feature, though perhaps there's some downside there I'm not really seeing in composability.

Any reason I shouldn't write this wrapper layer? Any reason you didn't go this route? I'd vastly prefer to write the classes.

@drarmstr
Copy link
Contributor

Recoil tries to provide some minimal pieces that gives us flexibility in building helpers and other abstractions on top to capture useful patterns. We are publishing some with the library in "Utils", we have more internally, and love to see others use the building blocks for their needs. I'm not exactly sure how your proposal is intended to work with the Recoil hooks for integrating with the current state, but am curious. While the documentation is still mostly a WIP, check out this example in the docs to get some flavor of composability: https://recoiljs.org/docs/guides/asynchronous-data-queries/#data-flow-graph

@davidmccabe
Copy link
Contributor

Would love to see what you build.

@natew
Copy link
Author

natew commented May 18, 2020

It would be really easy to write a hook for it like so:

const store = useRecoilStore(TodoListStore)

useEffect(() => {
  store.x = 1
}, [])

Let's see your example:

const currentUserIDState = atom({
  key: 'CurrentUserID',
  default: 1,
});

const userInfoQuery = selectorFamily({
  key: 'UserInfoQuery',
  get: userID => async ({get}) => {
    const response = await myDBQuery({userID});
    if (response.error) {
      throw response.error;
    }
    return response;
  },
});

const currentUserInfoQuery = selector({
  key: 'CurrentUserInfoQuery',
  get: ({get}) => get(userInfoQuery(get(currentUserIDState)),
});

const friendsInfoQuery = selector({
  key: 'FriendsInfoQuery',
  get: ({get}) => {
    const {friendList} = get(currentUserInfoQuery);
    const friends = [];
    for (const friendID of friendList) {
      const friendInfo = get(userInfoQuery(friendID));
      friends.push(friendInfo);
    }
    return friends;
  },
});

function CurrentUserInfo() {
  const currentUser = useRecoilValue(currentUserInfoQuery);
  const friends = useRecoilValue(friendsInfoQuery);
  const setCurrentUserID = useSetRecoilState(currentUserIDState);
  return (
    <div>
      <h1>{currentUser.name}</h1>
      <ul>
        {friends.map(friend =>
          <li key={friend.id} onClick={() => setCurrentUserID(friend.id)}>
            {friend.name}
          </li>
        )}
      </ul>
    </div>
  );
}

function MyApp() {
  return (
    <RecoilRoot>
      <ErrorBoundary>
        <React.Suspense fallback={<div>Loading...</div>}>
          <CurrentUserInfo />
        </React.Suspense>
      </ErrorBoundary>
    </RecoilRoot>
  );
}

Rewritten in store-style:

class UserStore {
  id = 1
  
  get info() {
    return new Promise(async res => {
      await myDBQuery({ userID: this.id })
      // ...
    })
  }

  get friends() {
     // similar pattern to get info()
  }
}

function CurrentUserInfo() {
  const userStore = useRecoilStore(UserStore)
  return (
    <div>
      <h1>{userStore.info.name}</h1>
      <ul>
        {userStore.friends.map(friend =>
          <li key={friend.id} onClick={() => userStore.id = friend.id}>
            {friend.name}
          </li>
        )}
      </ul>
    </div>
  );
}

function MyApp() {
  return (
    <RecoilRoot>
      <ErrorBoundary>
        <React.Suspense fallback={<div>Loading...</div>}>
          <CurrentUserInfo />
        </React.Suspense>
      </ErrorBoundary>
    </RecoilRoot>
  );
}

You could also do just async info() and have it resolve on call in render, either one is imo straightforward.

I feel like you guys are making the same mistake Redux did in not making this class-based: you're ignoring that programmers really like to program in the langauge and not learn libraries with lots of concepts.

Selector Family? It takes a nested function where you use special get/set functions? useSetRecoilState and useRecoilValue and a few others?

Just seems like you have way more conceptual burden, a lot more syntax, a lot more abstraction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants