facebookexperimental / Recoil

Recoil is an experimental state management library for React apps. It provides several capabilities that are difficult to achieve with React alone, while being compatible with the newest features of React.
https://recoiljs.org/
MIT License
19.61k stars 1.19k forks source link

request for different selector param names #398

Open balupton opened 4 years ago

balupton commented 4 years ago

in the getting started guide we have

function CharacterCount() {
  const count = useRecoilValue(charCountState);

  return <>Character Count: {count}</>;
}

which is fine, but we also have

const charCountState = selector({
  key: 'charCountState', // unique ID (with respect to other atoms/selectors)
  get: ({get}) => {
    const text = get(textState);

    return text.length;
  },
});

which requires you to juggle two different contexts for what get means, why not the following to be consistent with using a recoil state value (an atom)

const charCountState = selector({
  key: 'charCountState', // unique ID (with respect to other atoms/selectors)
  get: ({fromRecoilValue}) => {
    const text = fromRecoilValue(textState);

    return text.length;
  },
});

or if we want to use the atom terminology

const charCountState = selector({
  key: 'charCountState', // unique ID (with respect to other atoms/selectors)
  get: ({fromAtom}) => {
    const text = fromAtom(textState);

    return text.length;
  },
});

or just something as simple as get

const charCountState = selector({
  key: 'charCountState', // unique ID (with respect to other atoms/selectors)
  get: ({from}) => {
    const text = from(textState);

    return text.length;
  },
});
behnammodi commented 4 years ago

@balupton Last item it's very simple and clean 👌🏻

drarmstr commented 4 years ago

In fact, we are also looking for naming symmetry between the callbacks in the selector evaluation function and the Snapshot API used in useRecoilCallback().

behnammodi commented 4 years ago

@drarmstr In totally, you don't agree with this change?

cybervaldez commented 4 years ago

subscribe is also a pretty good one:

const charCountState = selector({
  key: 'charCountState', // unique ID (with respect to other atoms/selectors)
  get: ({subscribe}) => {
    const text = subscribe(textState);

    return text.length;
  },
});

since it does subscribe to the changes.

thegrumpysnail commented 4 years ago

Somewhat related, I've actually run into problems with get conflicting with import { get } from 'lodash/fp', so I have to do something like -

export const someSelector = selector({
  key: 'someSelector',
  get: ({ get: getRecoil }) => {
    const recoilValue = getRecoil(anotherSelector);

    return get('value', recoilValue);
  },
});

It's kind of gross, but what if you allowed users to configure their selector function for get / set names?

import {
  selectorCreator,
} from 'recoil';

const selector = selectorCreator({
  getFieldName: 'subscribe',
});

export const someSelector = selector({
  key: 'someSelector',
  get: ({ subscribe }) => {
    const recoilValue = subscribe(anotherSelector);

    return get('value', recoilValue);
  },
}); 

... or if you didn't want to do that, allow users to override the field name in the selector itself ...

export const someSelector = selector({
  key: 'someSelector',
  getFieldName: 'subscribe',
  get: ({ subscribe }) => { ... },
});

... or finally (and perhaps my favorite), you could just follow in useState's footsteps and just let users decide what to call their get function -

export const someSelector = selector({
  key: 'someSelector',
  get: ([ subscribe ]) => { ... },
});

Just throwing ideas out here - so far I love recoil and I'm excited to be using it.

drarmstr commented 4 years ago

As you point out, you can already rename get when destructuring if you have a conflict. This callback we do want to keep short and concise for the common-case as it is used a lot. We actually previously had a longer name for this, and it became a hassle. We also wouldn't want to use a tuple because we want to maintain flexibility for offering different callbacks and allowing users to choose which one(s) to use without dealing with unused parameters and encourage standardization of terminology in code snippets.

thegrumpysnail commented 4 years ago

Yeah, that's fair @drarmstr - it's easy enough to work around it, so it's not a deal breaker. I can understand wanting to keep the option for different callbacks open, although that could also be accomplished by creating a different get* property of the selector than expanding the number of callbacks available already, and keeping the main get function as simple as possible (depending on what you had in mind for new callbacks of course). - Thinking about it further, that doesn't really make much sense.

The reason for my suggestion was just that in my limited experience, I kept running into a problem where I would use the wrong get in different selectors, and so I had to keep remembering to destructure if I planned on using lodash's get. I could have just destructured in every selector for consistency, but I guess I wanted to keep in the spirit of recoil and use the library as it was intended.

cybervaldez commented 4 years ago

Wouldn’t subscribe be more appropriate though since we are subscribing to changes? And then we can have get for the times we want to take the current value of an atom but not subscribe to it?

drarmstr commented 4 years ago

It wouldn't be valid to get a dependency value and not subscribe when computing a selector value, as selector evaluation functions are idempotent given the dependency values.