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.

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

Jump to bottom

fix: update history when a state property is set to undefined #14

Merged
merged 9 commits into from
Apr 28, 2024
Merged

fix: update history when a state property is set to undefined #14

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
be83807
fix: update history when a state property is set to undefined
Apr 24, 2024
64ff0ac
use type for empty wip snapshot
Apr 24, 2024
3068c23
simplify test case
Apr 25, 2024
cbccfa1
add test case to check behavior with delete keyword
Apr 25, 2024
fe0d61c
move tests to vanilla suite
Apr 28, 2024
1239a3d
use symbol for empty history wip
Apr 28, 2024
9a8e550
avoid updating version
Apr 28, 2024
e881a74
remove unused import
Apr 28, 2024
59e6c6a
apply formatting
Apr 28, 2024
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
2 changes: 1 addition & 1 deletion packages/history-utility/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "valtio-history",
"version": "0.3.2",
"version": "0.3.3",
Copy link
Collaborator

Choose a reason for hiding this comment

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

you dont need to adjust this.. its automated when releasing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, rolled back

"author": "Daishi Kato",
"repository": {
"type": "git",
Expand Down
102 changes: 101 additions & 1 deletion packages/history-utility/src/__tests__/history-utility.react.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { StrictMode } from 'react';
import { fireEvent, render, screen } from '@testing-library/react';
import { useSnapshot } from 'valtio';
import { describe, it } from 'vitest';
import { describe, it, expect } from 'vitest';

import { proxyWithHistory } from '../';

Expand Down Expand Up @@ -319,4 +319,104 @@ describe('proxyWithHistory: react', () => {
);
});
});

describe('edge cases', () => {
it('should update history when a state property is set to undefined', async () => {
const initialState: { count: number; foo?: string } = {
count: 0,
foo: 'bar'
};

const state = proxyWithHistory(initialState);

function incrementCounter() {
state.value.count++;
}

function setValueToUndefined() {
state.value.foo = undefined;
}

const Counter = () => {
const snap = useSnapshot(state);
return (
<>
<div>count: {snap.value.count}</div>
<button onClick={snap.undo}>undo</button>
<button onClick={snap.redo}>redo</button>
</>
);
};

render(
<StrictMode>
<Counter />
</StrictMode>
);

await screen.findByText('count: 0');
expect(state.history.nodes.length).toBe(1);
expect(state.history.nodes[state.history.nodes.length - 1]?.snapshot.count).toBe(0);

incrementCounter();
await screen.findByText('count: 1');
expect(state.history.nodes.length).toBe(2);
expect(state.history.nodes[state.history.nodes.length - 1]?.snapshot.count).toBe(1);

incrementCounter();
setValueToUndefined();
await screen.findByText('count: 2');
expect(state.history.nodes.length).toBe(3);
expect(state.history.nodes[state.history.nodes.length - 1]?.snapshot.count).toBe(2);
});

it('should update history when a state property is deleted', async () => {
const initialState: { count: number; foo?: string } = {
count: 0,
foo: 'bar'
};

const state = proxyWithHistory(initialState);

function incrementCounter() {
state.value.count++;
}

function deleteValue() {
delete state.value.foo;
}

const Counter = () => {
const snap = useSnapshot(state);
return (
<>
<div>count: {snap.value.count}</div>
<button onClick={snap.undo}>undo</button>
<button onClick={snap.redo}>redo</button>
</>
);
};

render(
<StrictMode>
<Counter />
</StrictMode>
);

await screen.findByText('count: 0');
expect(state.history.nodes.length).toBe(1);
expect(state.history.nodes[state.history.nodes.length - 1]?.snapshot.count).toBe(0);

incrementCounter();
await screen.findByText('count: 1');
expect(state.history.nodes.length).toBe(2);
expect(state.history.nodes[state.history.nodes.length - 1]?.snapshot.count).toBe(1);

incrementCounter();
deleteValue();
await screen.findByText('count: 2');
expect(state.history.nodes.length).toBe(3);
expect(state.history.nodes[state.history.nodes.length - 1]?.snapshot.count).toBe(2);
});
});
});
20 changes: 13 additions & 7 deletions packages/history-utility/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,14 @@ export type HistoryNode<T> = {
updatedAt?: Date;
};

type EmptyWIP = Record<string, never>;
const EmptyWIP: EmptyWIP = {};
Copy link
Collaborator

@lwhiteley lwhiteley Apr 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i still have a problem with the semantics here so how about we use a Symbol and more standard naming convention

Suggested change
type EmptyWIP = Record<string, never>;
const EmptyWIP: EmptyWIP = {};
const EMPTY_WIP = Symbol('valtio-history-wip-empty');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I fixed the naming case and used a Symbol.


export type History<T> = {
/**
* field for holding sandbox changes; used to avoid infinite loops
*/
wip?: Snapshot<T>;
wip: Snapshot<T> | EmptyWIP;
Copy link
Collaborator

Choose a reason for hiding this comment

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

given we use this once

Suggested change
wip: Snapshot<T> | EmptyWIP;
wip: Snapshot<T> | typeof EMPTY_WIP;

/**
* the nodes of the history for each change
*/
Expand Down Expand Up @@ -134,7 +137,7 @@ export function proxyWithHistory<V>(
* - history.wip: field for holding sandbox changes; used to avoid infinite loops<br>
*/
history: ref<History<V>>({
wip: undefined, // to avoid infinite loop
wip: EmptyWIP, // to avoid infinite loop
Copy link
Collaborator

Choose a reason for hiding this comment

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

also ensure the others are renamed

Suggested change
wip: EmptyWIP, // to avoid infinite loop
wip: EMPTY_WIP, // to avoid infinite loop

nodes: [],
index: -1,
}),
Expand Down Expand Up @@ -230,7 +233,7 @@ export function proxyWithHistory<V>(
if (proxyObject.canUndo()) {
proxyObject.history.wip = proxyObject.clone(
proxyObject.history.nodes[--proxyObject.history.index]?.snapshot
);
) ?? EmptyWIP
Copy link
Collaborator

@lwhiteley lwhiteley Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm unsure what the expectation is. The issue says that the value is set to undefined and no history change is reflected.. but this is setting the value to an empty object when null or undefined is found.

Seems like a data integrity issue.

Why not just set the value externally from the lib to an empty object if that's what's required in that use case?

What is the expectation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm understandin correctly, the method shouldSaveHistory returns true when the value changed and when the new value is different than the history wip.

Since by default (when empty) the history wip is undefined, when setting a value to undefined the above method returns false and the history isn't updated. By setting the default history wip value to an empty object, when the value has changed and the history wip is empty the above method would return true, so setting a value to undefined would work as expected.

Using an empty object ensures that it is equal (===) only to itself, so even setting the new value to an empty object would work as expected, but that is not the use case I was trying to address.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still don't really get the point..if the value is undefined and you try to set it to undefined then there should be no history change..

But if it's set then that means it's not undefined.

And if it sees undefined then undefined will not be equal to wip..

So what am I missing?

In theory there is no benefit to changing it to an empty object.

Do your test cases pass without these changes?

Copy link
Contributor Author

@fabio-grieco fabio-grieco Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's not the value that is undefined but the history wip property.

The first test case fails, the second one (delete) succeeds. If you run the first test case without the changes, you can see that when invoking setValueToUndefined() a state value is set from "bar" to undefined and the history is not updated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks.

I'll check it in detail later

Copy link
Collaborator

@lwhiteley lwhiteley Apr 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

    it('should update history when property is deleted', async () => {
      const state = proxyWithHistory<
        Partial<{ prop0: number; prop1: number; prop2: number; prop3: number }>
      >({ prop0: 0, prop1: 1, prop2: 2, prop3: 3 });

      await Promise.resolve();
      expect(state.value.prop0).toEqual(0);
      expect(state.value.prop1).toEqual(1);
      expect(state.value.prop2).toEqual(2);
      expect(state.value.prop3).toEqual(3);
      expect(state.history.nodes.length).toEqual(1);

      delete state.value.prop0;

      await Promise.resolve();

      expect(state.value.prop0).toEqual(undefined);
      expect(state.value.prop1).toEqual(1);
      expect(state.value.prop2).toEqual(2);
      expect(state.value.prop3).toEqual(3);
      expect(state.history.nodes.length).toEqual(2);
    });

    it('should update history when property is set to undefined', async () => {
      const state = proxyWithHistory<
        Partial<{ prop0: number; prop1: number; prop2: number; prop3: number }>
      >({ prop0: 0, prop1: 1, prop2: 2, prop3: 3 });

      await Promise.resolve();
      expect(state.value.prop0).toEqual(0);
      expect(state.value.prop1).toEqual(1);
      expect(state.value.prop2).toEqual(2);
      expect(state.value.prop3).toEqual(3);
      expect(state.history.nodes.length).toEqual(1);

      state.value.prop0 = undefined;

      await Promise.resolve();

      expect(state.value.prop0).toEqual(undefined);
      expect(state.value.prop1).toEqual(1);
      expect(state.value.prop2).toEqual(2);
      expect(state.value.prop3).toEqual(3);
      expect(state.history.nodes.length).toEqual(2);
    });

i used these test cases without your changes to understand the issue a little better in isolation of any other property changes.

first test case passes and the second one doesnt.

so thanks i have a clearer understanding

Copy link
Contributor Author

@fabio-grieco fabio-grieco Apr 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Nice, I added your your test cases to the vanilla suite and removed mine from react suite.

proxyObject.value = proxyObject.history.wip as V;
}
},
Expand All @@ -247,9 +250,10 @@ export function proxyWithHistory<V>(
*/
redo: () => {
if (proxyObject.canRedo()) {
proxyObject.history.wip = proxyObject.clone(
proxyObject.history.nodes[++proxyObject.history.index]?.snapshot
);
proxyObject.history.wip =
proxyObject.clone(
proxyObject.history.nodes[++proxyObject.history.index]?.snapshot
) ?? EmptyWIP;
proxyObject.value = proxyObject.history.wip as V;
}
},
Expand Down Expand Up @@ -308,7 +312,9 @@ export function proxyWithHistory<V>(
const resolvedIndex = isLastIndex ? index - 1 : index + 1;
const resolvedNode = proxyObject.history.nodes[resolvedIndex];

proxyObject.history.wip = proxyObject.clone(resolvedNode?.snapshot);
proxyObject.history.wip = proxyObject.clone(
resolvedNode?.snapshot
) ?? EmptyWIP;
proxyObject.value = proxyObject.history.wip as V;

if (isLastIndex) proxyObject.history.index--;
Expand Down