Dynamic container implementation

[albertogasparin]

Implementation of #184. The new container type will be created as:

createDynamicContainer({
  matcher: (Store) => Store.tags?.include('foo'),
  onStoreInit?: (Store) => (stateApi, props) => ...,
  onStoreUpdate?: (Store) => (stateApi, props) => ...,
  onStoreCleanup?: (Store) => (stateApi, props) => ...,
  onPropsUpdate?: (Store) => (stateApi, props) => ...,
  displayName?: 'my-store',
});

This new path also offers the chance to support #186, with a more store-driven API. Callbacks init/update/clear are now bound to store instances, and we provide a clearer onPropsUpdate.

Similarly, #146 now becomes easier to manage and implement either via consumers code or by providing a second argument to onStoreBla that gives access to all stores initialised by the same container.

What is still not supported (and maybe never will) is multi container matching. As if a container matches a store, it will decide its scope (local, global) and if global then it won't match any other parent.

[albertogasparin]

Please review @theKashey and @anacierdem 🙏

[anacierdem]

Didn't have a chance to look into this in detail, but I will. Does this also resolve #97?

[albertogasparin]

Yes, via onStoreCleanup, but it's not available for createContainer (yet). I'm still thinking about how this new API and createContainer should relate, whenever they should coexist, or if I should merge them. Because after the rewrite, the difference is just the matcher. So maybe createDynamicContainer should just become:

createContainer((s) => true, options)

Where createContainer allows a Store or a matcher function as first argument. Don't wanna overcomplicate createContainer API or add breaking changes, but also adding a new creator where the underlying implementation is the same might end up costing more long term 🤔

[theKashey]

I think this addition is against the core premise of RSS, which is keeping independent atomic stores that are "contained" at various distinct boundaries. By making the boundary dynamic, it is essentially killing this opinionated view.

Stores are still independent, but you don't need to micromanage every one of them and as a result it might be easier to have more independent stores without the necessity to pull all of them to the top-level boundary of a "entity".

Strangely there are not many examples of similar pattern, but I think jotai-molecule is almost the same one, just a bit less "automatic" as you have to use it explicitly. Hoisted stores (https://github.com/reactjs/rfcs/pull/241) are also more than related.

[anacierdem]

Thank you for the references. I will give that rfc a read. I now think is is more important to improve the examples so that it is even clearer for the users.

[anacierdem]

I am still thinking about this in the back of my mind :)

How is this different from a single store where we keep both states but use them behind RSS selectors? It will have the same performance benefits and it will be clear that those states are contained together just by looking at the store implementation. The new dynamic container breaks this link, you need to find the decoupled dynamic container floating around to understand the behaviour. It may even cause an explosion of dynamic containers where you hunt for a weird store behaviour throughout the codebase to find relevant containers that may affect that specific store.

Think about many containers matching stores based on a state they contain and initializing some stuff on it, how can we find those containers in a big project?

[albertogasparin]

The trigger uses case was ensuring all RSS stores used inside a section of an app are scoped (no global leakage). If you have multiple teams creating stores, having a dynamic container that captures them all and scopes them automatically can reduce the risk of misconfiguration. Also it would also allow such scoping to happen without importing the stores, so without impacting bundle size.

Re global leakage, it seems like #190 might be a better discussion/solution.

you need to find the decoupled dynamic container floating around to understand the behaviour

This is a good callout. Maybe the matcher API is too powerful, and might accidentally match unwanted stores. If we move back to a more explicit API it might be more predictable, as we were talking in #184 :

createTagContainer('tag-foo'); // or family

Re #146 , my idea was to allow onStore* callbacks to receive all stores "collected" under the same tag and enable actions on them. Something like:

createTagContainer('theme', {
  onStoreInit: (store, { dispatchTo }) => () => {
    if(...) dispatchTo(ColorsStore, actions.doSomething()); // trigger actions on another store
  }
})

The reason this is possible here is because we can validate that the store called by dispatchTo will be scoped to that container. Doing it in other locations means we might not know which boundary the action will hit and cause random effects.

[anacierdem]

For multiple team setups, having a dynamic container really makes sense. This makes this clearly an "admin" feature. Because if handed directly to those teams, it is possible that they misuse the tool by using the dynamic container to manage their own stores, which would cause the logic to get scattered.

Maybe the matcher API is too powerful, and might accidentally match unwanted stores.

I don't think the problem is the power of the matcher, and the problem is not accidentally matching but rather intentionally creating such dynamic containers. Even in case of #146 I think the dynamic container will be hard to find around. On my proposed solution, the cross-store logic is contained in the definition of the store by means of actual "dependencies", which makes the intent of the store clear.

OTOH, a dynamic container represents a boundary where any store can be manipulated/managed and by definition does not belong to any particular store. This is both a strength (no need to import member stores) and a downside because it does not have enough restriction on its usage.

Maybe if we can find a way to be more explicit about the collection of stores in a specific boundary, we may be in much better shape. Even tags can be a too dynamic without changing the direction. I wonder if a unique Symbol per container would solve the issue when we approach it from a different perspective;

This is where we define the dynamic container:

// This couples the tag with the container, the only way to get captured by this container is importing
// `themeTag` from this module. It should be a lightweight import for the consumer.
export const [ThemingContainer, themeTag] = createTaggedContainer({
  ...
});

Then the actual stores reference the exported tag;

const Store = createStore({
  initialState,
  actions,
  tags: [themeTag]
});

This reverses the usage pattern. Now the central authority decides on the tags but the downside is everyone now is supposed to put the tag on their container, and we are back to square one with the configuration risk. Even in case of string tags, how are you planning to make sure all the stores have the same tag to capture? That still has the same configuration risk and is not very different than the unique tag pattern above?

[theKashey]

The last proposal also enhances discoverability - you no longer have to think which stores your Container will "match" as the information will be explicitly powered by dependencies.

I also think that the current implementation is "good to go" as it is generic enough to handle any case - from simple createContainer to a createTaggedContainer one. Let's just rename it into unsafe_ and... 🤷‍♂️ call it is day. It is the abstraction we need to build any other solution. And any other solution is not really a subject of this PR.

[albertogasparin]

I know that this has been a long convo, but given the large user base of RSS (and the fact that I don't wanna have to make new majors) I'm cautious on shipping things that will become a pain to unship.

For instance, I liked your last API proposal @anacierdem and while implementing it, I've discovered that it poses a circular dependency problem when we try to use the onStore*/onPropsUpdate methods.

// theming.js
import { Store as ColorsStore } from './colors.js';
export const [ThemingContainer, themingTag] = createTaggedContainer({
  onStoreInit: (store) => ({ ... }) => {
    if (store === ColorsStore) ...
  }
});

// colors.js
import { themeTag } from './theming.js'
export const Store = createStore({initialState, actions, tags: [themingTag] });

The Store needs the tag, but the tagged container needs the store to send the data/actions into the right one. Right now the "flow" is that containers depend on Stores. If we were to invert that relationship we would end up with devs inadvertently creating circles between container, stores and actions.

To avoid this, I can think of only 2 things:

1. Adopt the 2 APIs that @theKashey suggested , with createTag (that you put in a separate file).
2. Move all onStore*/onPropsUpdate methods to the stores themselves, so there is no reason for TaggedContainer to import anything. And at this point the container itself can be the "tag".

The latter sound compelling:

// theming.js
const ThemingContainer = createContainer();

// colors.js
import { ThemingContainer } from './theming.js'
export const Store = createStore({
  initialState, 
  actions, 
  containedBy: [ThemingContainer],
  onInit: () => (api, props) => ...
  onContainerUpdate: () => () => ...
});

however I question whenever there is an actual need for multi-container support, as that complicates the type safety around the props. What would be the use case? ( especially after createContainer looses most of its config) Another benefit of introducing containedBy (or similar) to Store is that it goes towards a solution for #190, where we can complain if a store with that set ends up being global.

[theKashey]

This is a good example of how long theoretical conversations do not stand a minute of a practical application.

From a cohesion point of view moving all hooks into store definition is the right approach... Now let's find a good reason not to do that. With the absence of onStoreUpdate on the container the only reason to have container configuration outside of store definition is creating more than one container with unique onInit hooks each. Is that a case?

PS: And containers still working as they are working, as there is no intent to break API. How old and the new API would interfere?

[theKashey]

Github is having really hard times

[anacierdem]

This is a good example of how long theoretical conversations do not stand a minute of a practical application.

Exactly, this is why I am totally ok with the unsafe api. That was something that just came to my mind, never tested in practice. Thank you @albertogasparin for giving it a go. I wish we had an unstable release that we can battle-test important API decisions 😄

I initially also thought createTag would make sense but arrived at createTaggedContainer while experimenting. containedBy on the other hand is a little restrictive, considering you need to modify the store to change the container boundary. Managing containers is already hard, this does not seem to help very much.

Still, having the handlers on the store rather than the container makes more sense to me as well. It would also make sure we can make sure stores are initialized regardless of the existence of a container. I am not exactly sure why containedBy needs to support multi-container though, the first matching container can be the "owner" of the data, or maybe I am missing something?

With the absence of onStoreUpdate on the container the only reason to have container configuration outside of store definition is creating more than one container with unique onInit hooks each.

The users should be able to tag the container with their custom prop and decide on what to do based on it if per-container initialization is a necessity. That was something we never felt the need for.

I am also thinking how can we have the two completely different API living together?

[albertogasparin]

containedBy on the other hand is a little restrictive, considering you need to modify the store to change the container boundary.

Looking at the ~100 createContainer() usages we have, 80-90% are created for testing/examples purposes. Those use onInit to bootstrap the store state with mock data. So if we solve that use case in some other way, I think swapping container boundaries might not be needed. One of the strength of current createContainer approach is that we don't risk accidentally importing want mock data, while having containedBy defined for testing purposes will promote that. If we were to sunset (or disincentivise) the current container-for-mocking use case, we need a solid alternative.

How old and the new API would interfere? I am also thinking how can we have the two completely different API living together?

First and easy option is exposing a new createSharedContainer API and keep it separate for now. On the other side, given they accept different arguments we might be able to enforce the configs via TS and runtime errors, eventually soft deprecating the old usage.

I am not exactly sure why containedBy needs to support multi-container though

Agree, we can probably release a first version with a single container support and then see if there is demand for more

[anacierdem]

80-90% are created for testing/examples purposes.

I can definitely say that in a particular repository of ours, almost all (if not all) of the containers are for actual initialization. Also we have found the ability to change the location of the containers after implementing the store, very useful so that we can decide on the data boundaries later. We have never used it for data mocking IINM :) So, I cannot provide further input there. OTOH, for those stores, we only ever had a single container that is strategically placed, so even we need to put them in containedBy it would just be a simple "list the container here" exercise.

[theKashey]

I am not exactly sure why containedBy needs to support multi-container though

it will create more problems than could possibly resolve. Let's do our best to create a pit success and not let developers to fall in some other hole.

80-90% are created for testing/examples purposes.

Which is something I think we need to address. The global nature of sweet-state makes it ease to use, but everything should have logical bounds - a standard example would be a boundary on application level (aka big context providers, or redux). That is "reusable component model" and predictability of environment we value relay for.

[albertogasparin]

Cool, thanks for this great convo, team! I think we have a better direction now.

• The createContainer API will evolve to allow being called without arguments. This will create a component that will capture only stores that explicitly declare they want to be captured by it.
• The createStore API will expand to allow two new attributes:
  • containedBy, accepting the container component created beforehand
  • hooks: { onInit, onUpdate, onDestroy, onContainerUpdate } These hooks will still require a container to be triggered (we will see if there is demand for onInit/onUpdate on containerises cases).
• Long term, we will recommend to use containedBy so we can throw an (async) error if we detect the container is not there, ensuring better predictability
• createContainer(Store, config) API will remain as an override mechanism, meaning in tests and some other circumstances such component will contain the store regardless of containedBy.

This extension of the API should allow progressive migration to containedBy in large products, without much risk. I'll raise another PR with the new code