Skip to content

Throw error when not contained #200

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.

Sign up for GitHub

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

Merged
albertogasparin merged 1 commit into from
May 18, 2023
Merged

Throw error when not contained #200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/api/store.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ createStore(config);

- `name` _(string)_: optional, useful for debugging and to generate more meaningful store keys.

- `containedBy` _(Container)_: optional, specifies the Container component that should handle the store boundary
- `containedBy` _(Container)_: optional, specifies the Container component that should handle the store boundary.
If set, RSS will throw an async uncaught error whenever the store is used without a container, given it might still work but it will likely cause unexpected behaviours.

- `handlers` _(object)_: optional, defines callbacks on specific events

Expand Down
21 changes: 21 additions & 0 deletions src/components/__tests__/integration.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,4 +470,25 @@ describe('Integration', () => {
expect(handlers1.onDestroy).toHaveBeenCalledTimes(1);
expect(handlers2.onDestroy).toHaveBeenCalledTimes(1);
});

it('should throw an error if contained store is used without container', async () => {
const rejectSpy = jest
.spyOn(Promise, 'reject')
.mockImplementation(() => {});
const Store1 = createStore({
name: 'one',
initialState: { todos: [], loading: false },
actions,
containedBy: createContainer(),
});

const Subscriber = createSubscriber(Store1);
render(<Subscriber>{() => null}</Subscriber>);
await actTick();

expect(rejectSpy).toHaveBeenCalled();
const [error] = rejectSpy.mock.calls[0];
expect(error).toEqual(expect.any(Error));
expect(error.message).toContain('should be contained');
});
});
5 changes: 3 additions & 2 deletions src/components/container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ function useContainedStore(scope, registry, props, override) {
// so we can provide props to actions (only triggered by children)
if (!containedStore) {
const isExisting = registry.hasStore(Store, scope);
const { storeState } = registry.getStore(Store, scope);
const { storeState } = registry.getStore(Store, scope, true);
const getProps = () => containerProps.current;
const actions = bindActions(Store.actions, storeState, getProps);
const handlers = bindActions(
Expand Down Expand Up @@ -328,7 +328,8 @@ function createFunctionContainer({ displayName, override } = {}) {
if (
!storeState.listeners().length &&
// ensure registry has not already created a new store with same scope
storeState === registry.getStore(Store, cachedScope).storeState
storeState ===
registry.getStore(Store, cachedScope, true).storeState
) {
handlers.onDestroy?.();
registry.deleteStore(Store, cachedScope);
Expand Down
16 changes: 13 additions & 3 deletions src/store/registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,18 @@ export class StoreRegistry {
this.defaultScope = defaultScope;
}

initStore = (key, Store) => {
initStore = (key, Store, fromContainer) => {
const { initialState, actions } = Store;

if (Store.containedBy && !fromContainer) {
Copy link

@theKashey theKashey May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it's not checking that container is the one indicated by containedBy, allowing any other container to be present.
Question - how I can disable or satisfy this condition in tests, where isolation might be not as required, or where I don't want to fix too many things at once?

Copy link
Collaborator Author

@albertogasparin albertogasparin May 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allowing any other container to be present

Only the ones targeting the same type! The store creation is triggered by a matching container, not any container.

how I can disable or satisfy this condition in tests

There are 2 patterns devs can use in tests:

  • use the containedBy container directly
  • use a new createContainer(Store), which also allows to override onInit/onUpdate

The latter might be better for storybooks/tests, but as long as there is a container that captures the store state somewhere in the tree, it will work without complaining.
This should also help pushing teams writing tests without accidentally leaking to global when containedBy is used.

Promise.reject(
new Error(
`Store ${Store.key} should be contained by a container but it is used globally. ` +
`While it might still work, it will likely cause unexpected behaviours.`
)
);
}

const storeState = createStoreState(key, initialState);
const boundActions = bindActions(actions, storeState);
const store = { storeState, actions: boundActions };
Expand All @@ -25,9 +35,9 @@ export class StoreRegistry {
return this.stores.has(key);
};

getStore = (Store, scopeId = this.defaultScope) => {
getStore = (Store, scopeId = this.defaultScope, fromContainer = false) => {
const key = this.generateKey(Store, scopeId);
return this.stores.get(key) || this.initStore(key, Store);
return this.stores.get(key) || this.initStore(key, Store, fromContainer);
};

deleteStore = (Store, scopeId = this.defaultScope) => {
Expand Down