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 🙏

[theKashey]

that's an interesting trick 👍

[theKashey]

can we add here a "marker" about this being a "contained store", not a global one?
That would greatly help with #190

[albertogasparin]

If we accept one of the proposed solutions (eg a new behavior attribute), then do we actually need it?
I was thinking RSS could check in the Registry whenever the store currently being initialised is following the correct behaviour, and if not throw an async error (so we don't break code).

[theKashey]

The change looks very natural.

[theKashey]

Should it be?

Suggested change

	{ globalRegistry = defaultRegistry, getStore = defaultRegistry.getStore }
	{ globalRegistry = defaultRegistry, getStore = globalRegistry.getStore }

[albertogasparin]

Does not really matter much, as both are coming from context at the same time. These defaults are there because I'm worried enzyme shallow might not populate context properly and blow up. See #57
I might actually check if it is still an issue with hooks...

[albertogasparin]

Checked in product and shallow enzyme seems fine. Removed the default fallback.

[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 🤔

[anacierdem]

I mainly went over the documentation and API in general as @theKashey is already looking at the implementation. I have a few concerns:

• I am not exactly sure how this will ease A better way to orchestrate multiple stores #146. While I understand that this new container type will enable creating a single container wrapping multiple stores, passing information between is still an issue IINM. It does not seem possible to call other store's actions or change its props from any of the event callbacks. Or similarly it is not possible to change some other store's state (which should be bad anyways).
• You are also right about as long as a similar functionality to onStoreInit and onStoreCleanup is not available for createContainer it won't help much. The only option is migrating to createDynamicContainer but only matching the relevant store to mimic createContainer, which would be a little weird.

Most importantly, 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.
Maybe I didn't exactly get how this is useful. One usecase coming to my mind is merging multiple containers at the same level into a single one, but what is the benefit here? While trying to isolate the different store's concerns from each other, I feel like we are introducing a mechanism to merge them in a weird way, which is not very useful.
The original idea is designing a store, and then deciding where to put those containers depending on the usage patterns. With this one, we design the usage pattern first. I agree that managing containers is hard but my gut tells me this is not the correct direction to improve it.

I don't object to it too much as it is not affecting the existing API, but I don't see the value here. I may be totally off, please help me understand if that's the case. I would be really happy to see some real-life scenarios. In the provided example, it is trivial to replace ThemingContainer with a component composed of two containers created by createContainer, I don't think it is worth this additional complexity.

[anacierdem]

I think we should include the function parameters here. Especially for this one, it is not clear how I will know which store instance is changing state? Just by looking at the documentation, it is not clear. Same for the other function configs as well.

[anacierdem]

Which onInit? Is that a typo?

[anacierdem]

How do I define the tags here? Need something in the docs IMO, currently only way to know is digging the example files.

[anacierdem]

Instead of duck typing the state, wouldn't be better to check the store against some reference. b/c the parameters to the action creator are not clear here, I am not sure how.

[anacierdem]

I believe this is an unrelated change. Ideally would be better if we can exclude from this PR.

[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 (reactjs/rfcs#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?

[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