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

Fix useHookstate store switchover #409

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 19 additions & 0 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"version": "1.0.0",
"configurations": [
{
"name": "Test Core",
"request": "launch",
"runtimeArgs": [
"nx",
"test",
"core"
],
"runtimeExecutable": "pnpm",
"skipFiles": [
"<node_internals>/**"
],
"type": "node"
}
]
}
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ This is the mono repository, which combine the Hookstate core package, extension

From the repository root directory:

- `npm install -f pnpm` - install pnpm tool
- `npm install -g pnpm` - install pnpm tool
- `pnpm install` - install node_modules for all packages

- `pnpm nx <script> <package>` - run script for a package as well as build dependencies if required, for example:
Expand Down
75 changes: 75 additions & 0 deletions core/src/__tests__/Complex.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -199,3 +199,78 @@ test('complex: should auto save latest state for unmounted', async () => {
expect(unmountedLink.field1.get()).toStrictEqual(2);
expect(result.current[0].get().field1).toStrictEqual(2);
});

test('scoped: should reinitialize when parent state changes', async () => {
const initialStore = hookstate({ a: 0 });
const newStore = hookstate({ a: 1 });

let renderTimes = 0;
const { result, rerender } = renderHook(({ source }) => {
renderTimes += 1;
return useHookstate(source).a;
}, { initialProps: { source: initialStore } });

expect(renderTimes).toBe(1);
expect(result.current.get()).toBe(0);

act(() => {
result.current.set(p => p + 1);
});

expect(result.current.get()).toBe(1);
expect(renderTimes).toBe(2);

rerender({ source: newStore });

expect(renderTimes).toBe(3);
expect(result.current.get()).toBe(1); // Should reinitialize to new store's value
});

test('should synchronize unsubscription and reinitialization when source/store changes', async () => {
const initialStore = hookstate(0);
const newStore = hookstate(42);

let renderTimes = 0;
const { result, rerender } = renderHook(({ source }) => {
renderTimes += 1;
return useHookstate(source);
}, { initialProps: { source: initialStore } });

expect(renderTimes).toBe(1);
expect(result.current.get()).toBe(0);

act(() => {
result.current.set(p => p + 1);
});

expect(result.current.get()).toBe(1);
expect(renderTimes).toBe(2);

rerender({ source: newStore });

expect(renderTimes).toBe(3);
Copy link
Owner

Choose a reason for hiding this comment

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

could you please extend the tests to make sure we can do state set action after the store is reinitialized? same for other tests you put.

expect(result.current.get()).toBe(42); // Should reinitialize to new store's value
});

test('local: should reinitialize when initial state changes', async () => {
let renderTimes = 0;
const { result, rerender } = renderHook(({ initialState }) => {
renderTimes += 1;
return useHookstate(initialState);
}, { initialProps: { initialState: { a: 0 } } });

expect(renderTimes).toBe(1);
expect(result.current.a.get()).toBe(0);

act(() => {
result.current.a.set(p => p + 1);
});

expect(result.current.a.get()).toBe(1);
expect(renderTimes).toBe(2);

rerender({ initialState: { a: 1 } });

expect(renderTimes).toBe(3);
expect(result.current.a.get()).toBe(1); // Should reinitialize to new initial state
});
15 changes: 9 additions & 6 deletions core/src/__tests__/Extension.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -191,12 +191,13 @@ test('extension: common flow callbacks', async () => {
expect(messages).toEqual([])
messages.splice(0, messages.length);

console.warn = jest.fn()
act(() => {
expect(() => result.current[0].f1.set(p => p + 1)).toThrow(
'Error: HOOKSTATE-106 [path: /0/f1]. See https://hookstate.js.org/docs/exceptions#hookstate-106'
);
result.current[0].f1.set(10)
});
expect(console.warn).toHaveBeenCalledWith(`Warning: HOOKSTATE-106: Attempt to set state when it is destroyed. [path: /0/f1]`)
expect(renderTimes).toStrictEqual(7);
expect(result.current.get()[0].f1).toStrictEqual(10);
expect(messages).toEqual([])
messages.splice(0, messages.length);
});
Expand Down Expand Up @@ -399,12 +400,14 @@ test('extension: common flow callbacks global state', async () => {
expect(messages).toEqual([])
messages.splice(0, messages.length);


console.warn = jest.fn()
act(() => {
expect(() => result.current[0].f1.set(p => p + 1)).toThrow(
'Error: HOOKSTATE-106 [path: /0/f1]. See https://hookstate.js.org/docs/exceptions#hookstate-106'
);
result.current[0].f1.set(p => p + 1)
});
expect(console.warn).toHaveBeenCalledWith(`Warning: HOOKSTATE-106: Attempt to set state when it is destroyed. [path: /0/f1]`)
expect(renderTimes).toStrictEqual(7);
expect(result.current.get()[0].f1).toStrictEqual(1);
expect(messages).toEqual([])
messages.splice(0, messages.length);
});
2 changes: 1 addition & 1 deletion core/src/__tests__/Primitive.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ test('primitive: global state created locally', async () => {
result.current!.set(p => p + 1);
});
expect(renderTimes).toStrictEqual(2);
expect(errorMessage).toStrictEqual("Error: Error: HOOKSTATE-111 [path: /]. See https://hookstate.js.org/docs/exceptions#hookstate-111")
expect(errorMessage).toStrictEqual("")
});

test('primitive: stale state should auto refresh', async () => {
Expand Down
41 changes: 26 additions & 15 deletions core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -609,9 +609,10 @@ export function useHookstate<S, E extends {} = {}>(
// warning: this is called twice in react strict mode
let store = parentMethods.store
let onSetUsedCallback = () => setValue({
store: store, // immutable
state: state, // immutable
source: value.source // mutable, get the latest from value
store, // immutable
state, // immutable
source: value.source, // mutable, get the latest from value,
parentMethods
})
let state = new StateMethodsImpl<S, E>(
store,
Expand All @@ -621,22 +622,26 @@ export function useHookstate<S, E extends {} = {}>(
onSetUsedCallback
);
return {
store: store,
state: state,
source: source
store,
state,
source,
parentMethods
}
};
const [value, setValue] = React.useState(initializer);
let [value, setValue] = React.useState(initializer);

if (value.store !== parentMethods.store || !('source' in value)) {
throw new StateInvalidUsageError(parentMethods.path, ErrorId.InitStateStoreSwitchover)
value.state.onUnmount()
value.parentMethods.unsubscribe(value.state);
value = initializer()
}

// TODO move to a class hide props on prototype level
// hide props from development tools
Object.defineProperty(value, 'store', { enumerable: false });
Object.defineProperty(value, 'state', { enumerable: false });
Object.defineProperty(value, 'source', { enumerable: false });
Object.defineProperty(value, 'parentMethods', { enumerable: false });

value.state.reconstruct(
parentMethods.path,
Expand Down Expand Up @@ -674,7 +679,7 @@ export function useHookstate<S, E extends {} = {}>(
let initializer = () => {
// warning: this is called twice in react strict mode
let store = parentMethods.store
let onSetUsedCallback = () => setValue({
let onSetUsedCallback = () => value.state.isMounted && setValue({
store: store, // immutable
state: state, // immutable
source: value.source // mutable, get the latest from value
Expand All @@ -692,10 +697,12 @@ export function useHookstate<S, E extends {} = {}>(
source: source
}
}
const [value, setValue] = React.useState(initializer);
let [value, setValue] = React.useState(initializer);

if (value.store !== parentMethods.store || !('source' in value)) {
throw new StateInvalidUsageError(parentMethods.path, ErrorId.InitStateStoreSwitchover)
value.state.onUnmount()
value.store.unsubscribe(value.state);
value = initializer()
}

// hide props from development tools
Expand Down Expand Up @@ -743,7 +750,7 @@ export function useHookstate<S, E extends {} = {}>(
let initializer = () => {
// warning: this is called twice in react strict mode
let store = createStore(source)
let onSetUsedCallback = () => setValue({
let onSetUsedCallback = () => value.state.isMounted && setValue({
store: store,
state: state,
})
Expand All @@ -759,10 +766,13 @@ export function useHookstate<S, E extends {} = {}>(
state: state
}
}
const [value, setValue] = React.useState(initializer);
let [value, setValue] = React.useState(initializer);

if ('source' in value) {
throw new StateInvalidUsageError(RootPath, ErrorId.InitStateStoreSwitchover)
value.state.onUnmount()
value.store.unsubscribe(value.state);
value.store.deactivate()
value = initializer()
}

// hide props from development tools
Expand Down Expand Up @@ -1056,7 +1066,8 @@ class Store implements Subscribable {
set(path: Path, value: StateValueAtPath): SetActionDescriptor {
if (this.edition < 0) {
// TODO convert to console log
Copy link
Owner

Choose a reason for hiding this comment

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

maybe remove TODO comment? and update web docs for this error code?

throw new StateInvalidUsageError(path, ErrorId.SetStateWhenDestroyed)
// throw new StateInvalidUsageError(path, ErrorId.SetStateWhenDestroyed)
console.warn(`Warning: HOOKSTATE-106: Attempt to set state when it is destroyed. [path: /${path.join('/')}]`)
}

if (path.length === 0) {
Expand Down