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

Non global states #190

Closed
theKashey opened this issue Mar 8, 2023 · 4 comments · Fixed by #200
Closed

Non global states #190

theKashey opened this issue Mar 8, 2023 · 4 comments · Fixed by #200

Comments

@theKashey
Copy link

continuation of #184

The main difference between SweetState and Context Provider is that the Context requires Provider to exists before Consumer. There is a case without Provider, in such case consumer will get the "default state" of context.

const context = createContext('default');

<Consumer> // 'default'
 <Provider value='overriden'>
  <Consumer> // 'overriden

This creates a lot of friction but also resolves many problems by making information flow unidirectional.

Resolving similar problems in a similar way is something I hope #184 can give us. One day. But I would like to have some support right now.


I would like to introduce temporal mitigation of a problem global states might cause by introducing an extra flag to the createStore making it strict non-global in the dev environment and throw if a boundary container is missing.

const store = createStore<Store, Actions>({
    initialState: {},
    actions: {        
    },
    name: 'my-store',
    isolated: true, // ⬅️ will throw on access to global store
});

There is no other intention for this feature except support refactoring and create "screaming tests" when something which might blow up will blow up. There is no goal to break anything in production, as there could be still use cases missed by tests.

@anacierdem
Copy link

anacierdem commented Mar 13, 2023

There is also another tangent to this: when there is an isGlobal container, RSS will execute the relevant actions (onInit, onUpdate etc.) but if you forget the container (or misplace the hook), it will silently continue using the non-initialized global instance. I think this will also detect container-less usages as once you set isolated to true, you will have to use a container as well. OTOH will only allow a global container, not a scoped one. So maybe we also need a contained: true which will throw on container-less store accesses.

From another viewpoint, one can argue that detecting the existence of a container is possible by additional logic at onInit. Is there a way to detect the same condition (if the instance is global) from the userland code?

@theKashey
Copy link
Author

That sounds interesting - distinguish global containers, local containers and container-less use cases.

contained: undefined, // or 'global'
contained: 'local', // local scope-less container
contained: 'scope', // local any scoped container
contained: true // any container, including global

Might be too much for one field, but this looks like a single property. Let's do the opposite and create 3 different options:

// requires container with a scope
scoped: true, // false
// disallow global usage
global: false
// requires any container
contained: true

All 3 makes a sense, but not simultaneously.

@albertogasparin
Copy link
Collaborator

albertogasparin commented Apr 11, 2023

Maybe something named expectContainer ? Because I feel like we want to catch problems when some hooks are used outside container but not sure what use case there is for "limiting" a store to be used only on scoped/local 🤔

However, I'm not a huge fun of blowing things up. If we proceed with #184 then it is quite simple to "log" things that end up being global but they should not.

As said in the opening message, this behaviour can be implemented with tag + global container.

const AppContainer = createGlobalContainer({
  capture: (StoreType) => !StoreType.tag.startsWith('global'),
  onStoreInit: (storeInstance, otherInitialisedStores) => { 
    // log exception and report store
  },
})

But I understand this will be less "harsh" than the store self check as it will not catch places where AppContainer is missing. Unless we add a global api to the Registry class:

defaultRegistry.onStoreInit((storeInstance, otherInitialisedStores) => { 
  // log exception and report store
})

That way we can directly look into the stores initialised in the global scope

@theKashey
Copy link
Author

Proposed solution:

  • extend createStore, add new optional properly - behavior/target/mode (🤷‍♂️)
createStore(   
    initialState: {},
    actions: {        },
    name: 'my-store',
    behavior: 'contained', // cannot be global. Writing or reading to the global store will throw an error
    behavior: 'global', // cannot be containerized. Explicitly global
    behavior: undefined, // uncontrolled
})

// alternatively utilize single boolean
createStore(   
    initialState: {},
    actions: { },
    name: 'my-store',
    isolated: true, // isolated
    isolated: false, // global
    isolated: undefined, // uncontrolled
});
  • in order not to worry much about the API above - expose constructs with better discoverability. Aka named factories:
- createLocalStore
- createGlobalStore
  • depending on friction adopting test - add extra command to disable isolation control in tests, at least for migration period. Something I don't think we need, but something I feel we might need.

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 a pull request may close this issue.

3 participants