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

fabio-grieco · 2024-04-24T21:47:49Z

Fixes #13

lwhiteley · 2024-04-25T03:46:44Z

packages/history-utility/src/__tests__/history-utility.react.spec.tsx

+        if (state.value.count === 1) {
+          state.value.foo = undefined;
+        } else {
+          state.value.foo = 'bar';
+        }
+        state.value.count++;


I'm not certain what this test is trying to prove.. all changes within the same execution scope will cause a single history change as far as I know. This includes increasing the counter.

Can you provide a simpler test that sets a value from defined to undefined.

You can also use the delete keyword to remove a property on the object as well

I would be interested to see both scenarios. And if they work or not without the changes made to the library

Ensure to add an await before the expectation is confirmed to simulate a change in execution context.

I simplified the test case and I added another test case to check the behavior with delete.

lwhiteley · 2024-04-25T03:49:05Z

packages/history-utility/src/index.ts

@@ -230,7 +233,7 @@ export function proxyWithHistory<V>(
      if (proxyObject.canUndo()) {
        proxyObject.history.wip = proxyObject.clone(
          proxyObject.history.nodes[--proxyObject.history.index]?.snapshot
-        );
+        ) ?? EmptyWIP


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?

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.

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?

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.

Thanks.

I'll check it in detail later

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

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

lwhiteley · 2024-04-25T06:48:21Z

packages/history-utility/src/__tests__/history-utility.react.spec.tsx

+      await screen.findByText('count: 0');
+
+      fireEvent.click(screen.getByText('inc'));
+      await screen.findByText('count: 1');
+
+      fireEvent.click(screen.getByText('inc'));
+      await screen.findByText('count: 2');
+
+      fireEvent.click(screen.getByText('inc'));
+      await screen.findByText('count: 3');
+
+      fireEvent.click(screen.getByText('undo'));
+      await screen.findByText('count: 2');
+
+      fireEvent.click(screen.getByText('redo'));
+      await screen.findByText('count: 3');
+
+      fireEvent.click(screen.getByText('undo'));
+      await screen.findByText('count: 2');
+
+      fireEvent.click(screen.getByText('undo'));
+      await screen.findByText('count: 1');
+
+      fireEvent.click(screen.getByText('undo'));
+      await screen.findByText('count: 0');
+
+      fireEvent.click(screen.getByText('inc'));
+      await screen.findByText('count: 1');
+
+      fireEvent.click(screen.getByText('undo'));
+      await screen.findByText('count: 0');
+    });


test title is : should update history when a state property is set to undefined

but there is no confirmation of what its trying to test

I added more meaningful expectations.

lwhiteley · 2024-04-27T04:53:11Z

packages/history-utility/src/index.ts

@@ -230,7 +233,7 @@ export function proxyWithHistory<V>(
      if (proxyObject.canUndo()) {
        proxyObject.history.wip = proxyObject.clone(
          proxyObject.history.nodes[--proxyObject.history.index]?.snapshot
-        );
+        ) ?? EmptyWIP


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

lwhiteley · 2024-04-27T04:54:42Z

packages/history-utility/src/index.ts

+type EmptyWIP = Record<string, never>;
+const EmptyWIP: EmptyWIP = {};


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');

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

lwhiteley · 2024-04-27T04:54:57Z

packages/history-utility/src/index.ts

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


given we use this once

Suggested change

wip: Snapshot<T> | EmptyWIP;

wip: Snapshot<T> | typeof EMPTY_WIP;

lwhiteley · 2024-04-27T04:56:02Z

packages/history-utility/src/index.ts

@@ -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


also ensure the others are renamed

Suggested change

wip: EmptyWIP, // to avoid infinite loop

wip: EMPTY_WIP, // to avoid infinite loop

lwhiteley · 2024-04-27T05:12:34Z

packages/history-utility/package.json

@@ -1,6 +1,6 @@
 {
  "name": "valtio-history",
-  "version": "0.3.2",
+  "version": "0.3.3",


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

Ok, rolled back

lwhiteley · 2024-04-28T13:55:52Z

you will need to run

pnpm nx format:write

otherwise looks good

fabio-grieco · 2024-04-28T14:07:03Z

Formatting applied 😊

lwhiteley · 2024-04-28T14:53:30Z

Thanks Fabio for finding and fixing this issue

I'll release it later

lwhiteley · 2024-04-29T03:00:01Z

release has been made
https://github.com/valtiojs/valtio-history/releases/tag/v0.3.3

Fabio Grieco added 2 commits April 24, 2024 23:43

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

be83807

use type for empty wip snapshot

64ff0ac

lwhiteley reviewed Apr 25, 2024

View reviewed changes

lwhiteley changed the title ~~Fix: update history when a state property is set to undefined~~ fix: update history when a state property is set to undefined Apr 25, 2024

lwhiteley reviewed Apr 25, 2024

View reviewed changes

Fabio Grieco added 2 commits April 25, 2024 14:06

simplify test case

3068c23

add test case to check behavior with delete keyword

cbccfa1

lwhiteley reviewed Apr 27, 2024

View reviewed changes

Fabio Grieco added 4 commits April 28, 2024 14:31

move tests to vanilla suite

fe0d61c

use symbol for empty history wip

1239a3d

avoid updating version

9a8e550

remove unused import

e881a74

apply formatting

59e6c6a

lwhiteley approved these changes Apr 28, 2024

View reviewed changes

lwhiteley merged commit ec38985 into valtiojs:main Apr 28, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fabio-grieco commented Apr 24, 2024

lwhiteley Apr 25, 2024 •

edited

Loading

fabio-grieco Apr 25, 2024

lwhiteley Apr 25, 2024 •

edited

Loading

fabio-grieco Apr 25, 2024

lwhiteley Apr 25, 2024

fabio-grieco Apr 25, 2024 •

edited

Loading

lwhiteley Apr 25, 2024

lwhiteley Apr 27, 2024 •

edited

Loading

fabio-grieco Apr 28, 2024 •

edited

Loading

lwhiteley Apr 25, 2024

fabio-grieco Apr 25, 2024

lwhiteley Apr 27, 2024 •

edited

Loading

lwhiteley Apr 27, 2024 •

edited

Loading

fabio-grieco Apr 28, 2024

lwhiteley Apr 27, 2024

lwhiteley Apr 27, 2024

lwhiteley Apr 27, 2024

fabio-grieco Apr 28, 2024

lwhiteley commented Apr 28, 2024

fabio-grieco commented Apr 28, 2024

lwhiteley commented Apr 28, 2024 •

edited

Loading

lwhiteley commented Apr 29, 2024

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

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

	wip: Snapshot<T> \| EmptyWIP;
	wip: Snapshot<T> \| typeof EMPTY_WIP;

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

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

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

Conversation

fabio-grieco commented Apr 24, 2024

lwhiteley Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lwhiteley Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabio-grieco Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lwhiteley Apr 27, 2024 • edited Loading

Choose a reason for hiding this comment

fabio-grieco Apr 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lwhiteley Apr 27, 2024 • edited Loading

Choose a reason for hiding this comment

lwhiteley Apr 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lwhiteley commented Apr 28, 2024

fabio-grieco commented Apr 28, 2024

lwhiteley commented Apr 28, 2024 • edited Loading

lwhiteley commented Apr 29, 2024

lwhiteley Apr 25, 2024 •

edited

Loading

lwhiteley Apr 25, 2024 •

edited

Loading

fabio-grieco Apr 25, 2024 •

edited

Loading

lwhiteley Apr 27, 2024 •

edited

Loading

fabio-grieco Apr 28, 2024 •

edited

Loading

lwhiteley Apr 27, 2024 •

edited

Loading

lwhiteley Apr 27, 2024 •

edited

Loading

lwhiteley commented Apr 28, 2024 •

edited

Loading