GeneralStore@3.0.0 with hooks

[aem]

This is an RFC / initial implementation of GeneralStore@3.0.0. The library will now fully support react@16 by adding a hooks-based api (GeneralStore#useStoreDependency). useStoreDependency only supports a single dependency, no compound dependencies, because the need for compound dependencies is gone. Whereas in older versions of GeneralStore users may have been required to pass multiple stores into connect to obtain all of the information required to render a component, users aren't limited in any way around how props can be passed into useStoreDependency so previous limitations around props, passing values between store derefs, or not being able to use local state don't exist at all. Instead, the new implementation allows us to use multiple stores independently, memoize the results of particular store calls, and pass arbitrary maps of props instead of needing to use the props of the current component as the props to the deref.

Upgrade Notes

1. This major version release is only compatible with react@^16.8. With the introduction of the hooks-based API, it's not safe to use new versions of GeneralStore with older versions of React. If you're using React <= 16.8.0 please use GeneralStore@2.5.1 or below.
2. GeneralStore@3 is written in TypeScript. The existing codebase, while compatible with the existing version of Flow and react@15, didn't work well with modern versions of Flow and React. Additionally, Flow's type system isn't refined enough to safely express the complex type relationships between all of the components in this library. TypeScript, on the other hand, has a more robust type system that allows us to be more confident in the code we ship with this library. The commits in this pull request were structured in a way that maintains git history and should be merged as a merge commit rather than a squashed commit to maintain that history.
3. Additionally, this version removes the following GeneralStore@2.x APIs:

• StoreDependencyMixin
  • Mixins have been deprecated by React for a while, so this API can be deprecated by GeneralStore as well
• connectWithState
  • This function was an antipattern and had a mental model that didn't make much sense. Now that local state can be passed into useStoreDependency trivially this API is unneeded and can be removed as well.

4. I removed the bower.json and relevant references. bower has been in maintenance mode for some time and it doesn't make sense to keep maintaining the library on the outdated package manager.

Example Usage

import React from 'react';
import GeneralStore from 'general-store';
import { Dispatcher } from 'flux';

const dispatcher = new Dispatcher();
GeneralStore.DispatcherInstance.set(dispatcher);
const INCREMENT = 'INCREMENT';
const StoreFactory = GeneralStore.defineFactory({})
  .defineGet(state => state)
  .defineGetInitialState(() => 1)
  .defineResponses({
    [INCREMENT]: state => state + 1,
  });
const store1 = StoreFactory.register(dispatcher);
const store2 = StoreFactory.register(dispatcher);
const increment = () => dispatcher.dispatch({ actionType: INCREMENT });
const dependency = {
  stores: [store1, store2],
  deref: () => store1.get() + store2.get(),
};

function UsesStore() {
  const __timer = React.useRef(null);
  React.useEffect(() => {
    __timer.current = setInterval(increment, 1000);
    return () => {
      clearInterval(__timer.current);
      __timer.current = null;
    };
  }, []);
  const value = GeneralStore.useStoreDependency(dependency);
  return <div className="App">{value}</div>;
}

export default UsesStore;

@colbyr

[colbyr]

const value = GeneralStore.useStore(dependency);

🔥🤩 so so good! so so exciting!

It's so interesting to look back on the evolution of the connect family, and be able to how the react apis / conventions of the time come out in these APIs. Obviously there's a lot of internal HubSpot context I've missed out on in the last year+. If you have the time to elaborate, I'm interested to learn some specifics for the sake of reflection/learning.

the new implementation allows us to use multiple stores independently, memoize the results of particular store calls, and pass arbitrary maps of props instead of needing to use the props of the current component as the props to the deref.

For the sake of discussion, I'd love to see one or two examples of what you imagine new usage patterns might look like!

connectWithState ... This function was an antipattern and had a mental model that didn't make much sense.

It's clear that connectWithState is obviated by this hooks api. It's so much simpler! If it turned out to be an antipattern, it sounds like it caused some trauma though. What problems did it cause?

Additionally, this version removes the following GeneralStore@2.x APIs: ... StoreDependencyMixin

BYYYEEEEEEEE 👋🎉

Additionally, Flow's type system isn't refined enough to safely express the complex type relationships between all of the components in this library.

I haven't worked with typescript enough to know what you mean. Would you mind going into what specifically becomes possible in Typescript? I trust you I'm just curious.

[Phoenixmatrix]

I haven't worked with typescript enough to know what you mean. Would you mind going into what specifically becomes possible in Typescript? I trust you I'm just curious.

I'd be curious too. While there's a lot to love with TS and is very likely a better choice going forward for many reasons, the power of the type system is rarely where I'd expect TS to be a slam dunk when comparing the two.

[aem]

@colbyr stack order for answering these questions:

Would you mind going into what specifically becomes possible in Typescript? I trust you I'm just curious.

The biggest advantage we have now (which I didn't do yet but will do in a separate PR - wanted to avoid too much churn here) is TypeScript can express the relationship between two types, so calculate could be typed as:

function calculate<D extends DependencyMap, Props>(dependencies: D, props?: Props) => { [K in keyof D]: any }

which would allow us to verify that the resultant object has the same keys as the dependency map and makes accessing those values typesafe. I couldn't find a way to do that in Flow (although my Flow experience is much more limited so maybe I just couldn't find the right construct). Either way I think the larger community built around TS will also be an advantage - more resources present and better tooling exists for TS than Flow in my experience.

I also kept running into issues using Flow with hooks. The types for hooks were behaving very weirdly and didn't play well with refs at all. I was unable to type refs well so I ended up having to just use * for the generic type of the refs which didn't feel good.

It's clear that connectWithState is obviated by this hooks api. It's so much simpler! If it turned out to be an antipattern, it sounds like it caused some trauma though. What problems did it cause?

No specific problems, but we ran into a few different cases where state had to be lifted into a wrapper component because we needed a local state value in a deref. This makes that disappear.

For the sake of discussion, I'd love to see one or two examples of what you imagine new usage patterns might look like!

The biggest place where we can simplify a ton of logic is customizing columns in the index pages. Currently we use connectWithState to combine locally changed columns and the state of the columns in the store. Now we can simply pass those columns into useStore - useStore(ColumnsDependency, { localColumns });

[Phoenixmatrix]

There are various utility types with Flow to handle these cases. They're just poorly documented but some (not all!) are here. While there are a few things TS can do that Flow can't, it's usually the other way around.

The community size and ease of use is really the argument for TS.

[aem]

Fair enough. Maybe just didn't do enough digging to get that dependent types code working properly. Regardless, still also had problems typing hooks properly (although that's something that might've been solved better by those utility types had I known about them before the migration.

[zdhickman]

a8659c4 looks really clean 👍

[colbyr]

Yeah today's flow has a lot more utility types than were available at the time most of this code was written. This was also the first real flow I ever wrote so there was some learning going on 🙃 I like flow, but I think the most important thing moving forward is that the folks maintaining this can understand the type checker. If y'all are in on TypeScript, do it 👍

No specific problems, but we ran into a few different cases where state had to be lifted into a wrapper component because we needed a local state value in a deref. This makes that disappear.

Word 👌

The biggest place where we can simplify a ton of logic is customizing columns in the index pages. Currently we use connectWithState to combine locally changed columns and the state of the columns in the store. Now we can simply pass those columns into useStore - useStore(ColumnsDependency, { localColumns });

That's a pretty compelling use case.

---

One thing I'd encourage you to look into is what happens when a component has two+ separate hook dependencies on the same store.

// this is a contrived example, but iirc it was pretty common in practice
const contact = useStore({stores: [ContactStore], deref: () => ContactStore.get('123')});
const relatedContacts = useStore({stores: [ContactStore], deref: () => ContactStore.get(['456', '789'])});

I'm not sure it's very explicit in the code but because connect and StoreDependencyMixin wrapped all the dependencies for a component when any change happened we were able wait until all values had resolved (using dispatcher.waitFor) and hopefully perform exactly 1 component update per dispatch.

I haven't kept super-up-to-date on fiber/hooks stuff, but naively I could imagine the example above might cause the component to re-render 2 times in rapid succession. It could also be that that isn't a big deal. You'd probably know better than me at this point 😜

[aem]

TypeScript is what's coming next in the pipeline internally, so I think that's what our teams will be comfortable with moving forward, which will inevitably come in handy if I'm not here to maintain this anymore :)

I did look into re-renders of multiple subscriptions. Here we actually benefit from React batching setState calls - since the subscription handlers are called in the same event loop, if both calls update that the same time then internally they'll get batched together, it's considered a single state transition. If the action is async and the stores update at separate times there will be two state transitions, but that's how connect works anyway.

I did just realize, however, that on each state transition here we're re-rendering the component even if the deref's return value is equal to the previous value. I'm going to update the hook to do the state calculations separate from the setDependencyValue call to allow us to compare the current and next values, only firing the state transition if the final value is new. Seems like an easy under-the-hood perf optimization.

[aem]

the hook has been renamed to useStoreDependency, and a performance test has been added to ensure state transitions only occur when the computed state value has changed (compared via strict equality).

[colbyr]

Nice! I like when react does stuff for you 👌

LGTM

[aem]

under the advisement of @Phoenixmatrix I migrated the build to Rollup. it produces better output, and shaves about 5kb off the minified bundle (down to about 20kb now). @henryqdineen mind taking a look at the rollup config? I largely copied from existing configs in some of the bender repackages, but wanna make sure I'm not doing anything super wrong here.

if all seems to be good with the build we should be good to ship this tomorrow/wednesday

[bobc7i]

👍

[aem]

@colbyr looking to merge/publish this today. would you be able to add npm user amarkon as a collaborator on the package so i can publish 3.0.0 once we're ready?

[henryqdineen]

rollup config looks 👍

[henryqdineen]

Have you considered enabling eslint-plugin-react-hooks?

Locally I get these warnings:

  64:6  warning  React Hook React.useEffect has a missing dependency: 'props'. Either include it or remove the dependency array                                                            react-hooks/exhaustive-deps
  77:6  warning  React Hook React.useMemo was passed a dependency list that is not an array literal. This means we can't statically verify whether you've passed the correct dependencies  react-hooks/exhaustive-deps
  77:6  warning  React Hook React.useMemo has missing dependencies: 'dependency', 'dependencyValue', and 'props'. Either include them or remove the dependency array                       react-hooks/exhaustive-deps

[aem]

hm. i thought i did have it enabled, guess not. i'll update that now

[colbyr]

@aem done 👍

[henryqdineen]

could this be setDependencyValue(calculate(dependency, props)); because of https://reactjs.org/docs/hooks-reference.html#bailing-out-of-a-state-update

[aem]

unfortunately not. it causes an infinite loop

[henryqdineen]

ok interesting. hooks are confusing!

[aem]

gonna squash some commits to make the history a lil more sane

[henryqdineen]