Enhancement: get informed about insertion/removal of properties

[rozek]

First of all: thank you very much for your marvellous work!

It looks brilliant - but lacks a feature I currently need: I want to get informed whenever a new property is added to an observed object.

After looking into your code, I quickly hacked the required changes in a way I can live with - but these changes are neither properly tested nor completely free of side effects (I'm currently using a "helper property" with a literal key rather than a JavaScript symbol - I did that in order not to touch too many parts of your code)

If you are interested, just have a look into my fork - perhaps, you'll come up with a better (or more professional) solution?

[luisherranz]

Can you show me an example of the syntax that you are trying to use? Is it something like this?

const store = deepSignal({ a: 1 });

effect(() => {
  for (const property in store) {
    console.log(`${property}: ${store[property]}`);
  }
});

store.b = 2;

[rozek]

Almost, I would say, but you will have to create a binding to the KeyListSymbol like so

import { deepSignal,KeyListSymbol } from 'deepsignal'

const store = deepSignal({ a: 1 });

effect(() => {
  store[KeyListSymbol]
  for (const property in store) {
    console.log(`${property}: ${store[property]}`);
  }
});

store.b = 2;

Just accessing store[KeyListSymbol] is sufficient, but if you prefer, you may also define a dummy variable or constant

PS: I just tried that example and it turns out that - while the new property is very well recognized - the loop does not mention the new key b although it can be accessed and delivers the expected value...

PS 2: the proxy, however, contains the new property and Object.keys(store) lists b as well (which is what I currently need). Nevertheless, it's very unpleasant that the iterator does not work properly

PS 3: a real non-enumerable JavaScript symbol would be much better, but this is what I was able to hack together this afternoon...

[rozek]

I've just added some code for the ownKeys Proxy method which eliminates my internal KeyListSymbol . Now Object.keys(store) produces the expected output...

...but the iterator still does not mention b

[rozek]

according to StackOverflow, trapping for...in seems to be difficult if not impossible...

[rozek]

well, I've made some more changes which now also deal with key in object and Object.getOwnPropertyDescriptor - they all work fine, but the for ... in loop doesn't.

  console.log(Object.keys(store))

  console.log('a in store','a' in store)
  console.log('b in store','b' in store)
  console.log('c in store','c' in store)
  console.log('* in store',KeyListSymbol in store)

  console.log('a',Object.getOwnPropertyDescriptor(store,'a'))
  console.log('b',Object.getOwnPropertyDescriptor(store,'b'))
  console.log('c',Object.getOwnPropertyDescriptor(store,'c'))
  console.log('*',Object.getOwnPropertyDescriptor(store,KeyListSymbol))

[rozek]

ok, I got it: I simply had to change the KeyListSymbol property after setting or defining the new property...

I.e., the for...in loop works (in principle), but the effect argument is called too often (twice upon setting store.b

PS: setting a new property internally invokes defineProperty - by updating the KeyListSymbol property in that method only I got rid of the extra effect invocation

[luisherranz]

I made this PR which should add support for things like for...in or Object.keys():

• Add support for the ownKeys trap #26

[luisherranz]

Released as 1.3.2, it would be great if you could test that it works 🙂

[rozek]

Good morning!

I found some minutes to test the new version - but, surprisingly, it does NOT work (don't yet know why)

Here is my code

  import { deepSignal } from 'deepsignal'

  const store = deepSignal({ a:1 })

  effect(() => {
console.log('------')
    for (const property in store) {
      console.log(`${property}: ${store[property]}`)
    }
  })

console.log('----')
  store.b = 2;
console.log('----')

  console.log('store',store)

which produces the following output:

------
a: 1
----
----
store Proxy(Object) {a: 1, b: 2}

Thus,

• the effect is invoked when defined,
• but not when store.b is set,
• however, the store finally contains the new property b

[rozek]

what I found out so far:

• the code itself works as intended (objToIterable.get(target).value++ is invoked)
• but that does not recalculate the effect body

However, family is calling and I'll have to postpone further investigations until tomorrow...

[rozek]

Ok, I got some more minutes and found a solution:

just change the ownKeys trap to

	ownKeys(target: object): (string | symbol)[] {
		if (!objToIterable.has(target)) objToIterable.set(target, signal(0));
		objToIterable.get(target).value = objToIterable.get(target).value
		return Reflect.ownKeys(target);
	},

a simple objToIterable.get(target).value is not sufficient (perhaps the compiler optimizes it away?)

But now everything works as intended

[luisherranz]

perhaps the compiler optimizes it away?

It removes the .value for some reason.

ownKeys: function (e) {
  return s.has(e) || s.set(e, t.signal(0)), s.get(e), Reflect.ownKeys(e);
},

What I don't understand is why the tests passed fine. They are supposed to run against the production build as well.

Anyway, thanks for checking it! I'll fix it on Monday.

[rozek]

My own fork is now based on your version 1.3.2 and includes both the bug fix mentioned above and the modifications I need for myself (for defineProperty and a property named $)

[luisherranz]

Cool, thanks.

[luisherranz]

Hey @rozek, can you test the latest release?

• https://github.com/luisherranz/deepsignal/releases/tag/deepsignal%401.3.3

for defineProperty and a property named $

Feel free to open new issues for those to explain the use case of each.

[rozek]

Thank you vey much for your effort!

I've merged your changes and built again - now I can confirm, that insertion and removal of properties are reported once the "ownKeys" trap has been activated.

[luisherranz]

Great, thanks!

[rozek]

As suggested, I've opened two new issues for the enhancements I need