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

Add test for Subscribe to nested value optimization #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lostpebble
Copy link

Was running into issues with this in my project (tree updating even though I've applied the optimization and the nested value isn't changing) so decided to create a test to see if it was functioning correctly.

Turns out it is functioning fine and there must be a problem from my side (still to be found).

But in any case, its another test case for the library which never hurts.

@lostpebble
Copy link
Author

So... turns out when I forked and built Bey for myself now and then linked it with yarn link - my project is working! 🙂

So I guess there are some changes relating to this optimization that haven't been pushed to a release version yet?

@lostpebble
Copy link
Author

Just to confirm, I created a repo with the exact same tests, but run on the current distribution version of bey:

https://github.com/lostpebble/bey-dist-test

You'll see the optimization / state selection stuff failing there.

Looking at the code directly, it seems that shallowEqual is not actually being used in the latest release version.

@bewildergeist
Copy link

Yeah, it appears that the built release version is not up to date with the source. The reset() method added in a0bcbda is also not present in the released 1.1.0 even though that release was tagged later, in adcd975.

@lostpebble
Copy link
Author

@jamiebuilds, would really appreciate a rebuild and publish so I don't have to do my symlinking solution to use bey at the moment.

This is actually a pretty huge bug that's slipping through at the moment for users (no optimization as described in the Readme).

@jamiebuilds
Copy link
Owner

Apologies, this project was mostly an experiment in API design which is why I never promoted it anywhere. I've been thinking a lot about the intersection of Unstated & Bey & React Hooks and trying to come up with a better API.

@bewildergeist
Copy link

@jamiebuilds So you'd recommend forking it or just copying bey.js (short and concise as it is) into a local project? Or switching to Unstated?

This just has a really simple and easy-to-grok API. But I can see that Hooks will enable some neat new ways of getting state with less <Subcribe/> wrapping. Will be excited to see what you come up with.

But in the meanwhile, it would be cool if you could cut a new npm release so the distributed code matches the 1.1.0 in the repo 😁

@sam-rad
Copy link

sam-rad commented Nov 6, 2018

@bewildergeist until the useContext is fixed in #14110 you can create a custom hook useBey and use it in place of <Subscribe />.

export const useBey = (to, on = s => s) => {
  const selection = on(to.get());
  const [cache, setCache] = useState(selection);

  const _update = nextAtom => {
    const newCache = on(nextAtom);
    !shallowEqual(cache, newCache) && setCache(newCache);
  };

  useEffect(
    () => {
      to.on(_update);
      return () => to.off(_update);
    },
    [on]
  );

  return selection;
};

export const Sub = ({ to, on = s => s, children }) => {
  const selection = on(to.get());
  const [cache, setCache] = useState({});
  useEffect(
    () => {
      to.on(setCache);
      return () => to.off(setCache);
    },
    [on]
  );

  return useMemo(() => children(selection), [selection]);
};

and use it like so:

const UseWithHook = props => {
  const user = useBey(atom, s => s.user);
  return <div>{user.name}</div>;
};

const UseWithSub = props => {
  <Sub to={atom} on={s => s.user}>
    {user => <div>{user.name}</div>}
  </Sub>
};

Here is the codesandbox

By the way, @jamiebuilds bey is fantastic.

@lostpebble
Copy link
Author

@jamiebuilds no worries. I understand that this project is a bit of an experiment. Albeit, it's one that I do really like in its simplicity and usability.

I've taken it and run with it in some of my newer projects, so would still really like a rebuild and publish if possible? Just a minor patch release, while you continue to ponder on the future of this problem with hooks etc.

I'm actually not sure how much easier you could make it, but I'll be very happy to be proved wrong! Looks like the hook that @sam-rad created here is a good direction - though I'd still like the option of the <Subscribe/> component to drop parts of my state into the React tree without needing to create a hook for it.

@davej
Copy link

davej commented Jan 27, 2019

Just an FYI, for anyone running into this. @lostpebble published bey-fix on npm, which I am using.

@lostpebble
Copy link
Author

Hey everyone, just thought I'd let you know that I've taken the lessons learned here and created something new - pullstate.

bey was great, but seems like the author views it more as an experiment than a real project at the moment.

Would love if you'd give it a go and give some feedback! It uses hooks (but also has a <Subscribe/>-type component) and has server-side rendering capabilities, as I required it (I had built my own server-rendering utilities over bey as well).

@davej
Copy link

davej commented Feb 17, 2019

@lostpebble Interesting. Regarding the API, why not allow something like this instead?

import { Store } from "pullstate";

export const { useStoreState, update, InjectStoreState } = new Store({
  theme: {
    mode: EThemeMode.DARK,
  },
  message: `What a lovely day`,
});
import { useStoreState, update } from "./stores/UIStore";

const App = () => {
  const theme = useStoreState(s => s.theme);

  return (
    <ThemeProvider theme={theme}>
      <button
        onClick={() => {
          update(s => {
            s.theme.mode = theme.mode === EThemeMode.DARK ? EThemeMode.LIGHT : EThemeMode.DARK;
          });
        }}
      >
        Switch it up!
      </button>
    </ThemeProvider>
  );
};

@lostpebble
Copy link
Author

Hey @davej, thanks for checking it out!

So the API you describe isn't far from how it works currently. But I didn't go exactly that route for a couple reasons:

  • Destructing classes can lead to unexpected behaviour - its better to keep them contained.
  • Namespacing is a powerful readability tool, and prevents scope clashing:
import { useStoreState, update } from "./stores/UIStore";
import { useStoreState, update } from "./stores/UserStore";

I need to update the docs, but there is actually already a convenience update() method on the Stores:

import { UIStore } from "./stores/UIStore";
import { useStoreState } from "pullstate";

const App = () => {
  const theme = useStoreState(UIStore, s => s.theme);

  return (
    <ThemeProvider theme={theme}>
      <button
        onClick={() => UIStore.update(s => {
            s.theme.mode = theme.mode === EThemeMode.DARK ? EThemeMode.LIGHT : EThemeMode.DARK;
        })}
      >
        Switch it up!
      </button>
    </ThemeProvider>
  );
};

So, I think this mostly achieves what you were looking for? Just that it still keeps your stores "namespaced", which I feel is a little more readable when things get slightly more complex.

Potentially, there could be an extra convenience method like you describe here, namespaced on the Store as well:

const theme = UIStore.useStoreState(s => s.theme);

Instead of

const theme = useStoreState(UIStore, s => s.theme);

But I think I prefer the readability and composition of the second one (it feels more "hooksy"). I'm open to discuss it though! Should maybe move that to the project though https://github.com/lostpebble/pullstate

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

Successfully merging this pull request may close these issues.

5 participants