Mutables need batching in some situations

[titoBouzout]

Describe the bug

Mutables behave in unexpected ways in some situations

In the cats example, pop() doesn't trigger an update
https://playground.solidjs.com/?hash=-1685740014&version=1.3.16

However, if you batch it, it works
https://playground.solidjs.com/?hash=1329499387&version=1.3.16

When doing push() it doesn't need batching and works as expected
https://playground.solidjs.com/?hash=-445597428&version=1.3.16

Lack of batching when in use with reconcile triggers unnecessary re-renders
https://playground.solidjs.com/?hash=-1814354602&version=1.3.7

Not sure if this will be related to this, but working with arrays doesn't trigger an update
https://playground.solidjs.com/?hash=-1939954988&version=1.3.16

Your Example Website or App

https://playground.solidjs.com/

Steps to Reproduce the Bug or Issue

Expected behavior

Screenshots or Videos

Platform

Additional context

[rturnq]

Ultimately the issue here is the array ends up with a hole at the end which throws an error when you access name and id. Mutables use stores under the covers which have the same issue if you do something like

const [store, setStore] = createStore({
  list: ['a', 'b', 'c']
})

setStore('list', 1, undefined)

I think the fix here might actually be simple. The deleteProperty proxy trap on mutables already handles the update from pop and tells the store to remove the property. The problem is store's setProperty uses delete which for arrays leads to a hole. In the above example list will look like ['a', empty, 'b']. We already have some special handling for arrays here so it seems like for them, checking if property is an index (number or string) and using splice should solve the issue.

[rturnq]

Making this change does mean you wouldn't be able to set an item in an array to undefined as it would splice it out instead. To be fair, the current implementation isn't ideal here either as the deleted item isn't iterated with things like Array.prototype.map. Maybe we need a special symbol for deletes that would be used internally by mutables while for store setters people could continue using undefined to remove non-array-item properties and finally doing something like setStore('list', 1, undefined) would actually set that array item to undefined.

[ryansolid]

Yeah I don't know I like that. I'm gathering the reason batch works is in the end the proxy sets the length but not as it goes. The real problem is the batching I gather. If there is a clever fix for that it may be the best option.

[rturnq]

Yeah, this is pretty interesting. I would have assumed array operations would be atomic but with proxies we can interrupt them and see how they are implemented. For instance pop removes the last item then sets the length and shift (as the name implies) copies all the elements one position down, removes the last item and then sets the length. We get notified for each of these steps.

My previous though only sort of solved it in a round about way but you're right it doesn't remove the intermediate notifications which are wasteful at best and otherwise glitchy. New idea: In the get proxy trap we check if the value is a function and part of the Array.prototype if it is we return a new function that calls the requested function in a batch.

if (typeof value === 'function' && Array.prototype[property] === value) {
  return (...args) => batch(() => value.apply(receiver, args))
}

[titoBouzout]

Posted something similar in discord, I got the prototype wrapped in batch, but still only works sometimes, if you press pop twice the second time gives an error from mapArray
https://playground.solidjs.com/?hash=-46883494&version=1.3.16

[rturnq]

@titoBouzout I think your example is just missing returning the result of the wrapped function from within the batch which appears to prevent something from getting a value back from a push.

[titoBouzout]

Ah yeah, thank you. Updated example https://playground.solidjs.com/?hash=805667632&version=1.3.16
It's a bit weird, applies the proxy to the unwrapped __proto__ and the function is applied to the mutable array. Not sure if helpful.

[edemaine]

New idea: In the get proxy trap we check if the value is a function and part of the Array.prototype if it is we return a new function that calls the requested function in a batch.

This seems like a good idea to me. Alternatively, we could have a white-list of certain operations (e.g. push, pop, splice, etc.) and possibly of types (Array, Set, Map, etc.) that automatically get wrapped in batch. But given that only wrappable objects get wrapped anyway, it's probably fine to do it in all cases?

[titoBouzout]

Mutable issues in 1.4

1. In top level this will work

const mutable = createMutable({ roomID: 'lobby' })
console.log(mutable.roomID) // prints 'lobby'
mutable.roomID = 'soemthing else'
console.log(mutable.roomID) // GOOD: prints 'soemthing else'

If you do onMount, it won't work

onMount(() => {
	const mutable = createMutable({ roomID: 'lobby' })
	console.log(mutable.roomID) // prints 'lobby'
	mutable.roomID = 'soemthing else'
	console.log(mutable.roomID) // BAD: will print lobby
})

2. This one I can't reproduce for some reason, but I see this on the site and cant come up with an example. Basically, I end with an HTML attribute like this attribute-name="(...args) => batch(() => Array.prototype[property].apply(receiver, args))"

function patchMutable(mutable, key, data) {
	if (mutable[key].id !== data.id) {
		mutable[key] = data
	} else {
		modifyMutable(mutable[key], reconcile(data))
	}
}

const mutable = createMutable({
	room: { id: 'lobby', users: [1, 2] },
})

// correctly will set mutable-length-original="2"
document.body.setAttribute(
	'mutable-length-original',
	mutable.room.users.length,
)

patchMutable(mutable, 'room', { id: 'lobby', users: [] })

// correctly will set mutable-length-patched="0"
document.body.setAttribute(
	'mutable-length-patched',
	mutable.room.users.length,
)

// here is the problem 
socket.on('room', data => {
 	patchMutable(state, 'room', data)
 
	state.roomID = state.room.id

	document.body.setAttribute('room', state.room.id)

	// will set user-count="(...args) => batch(() => Array.prototype[property].apply(receiver, args))"
 	document.body.setAttribute('user-count', state.room.users.length)
})

3. The createEffect change and the problems noted above stormed the site we're working on. So is hard to tell why something don't work. However, I noted that on this line, maybe there is some mistake? because when I change it, stuff works a bit better. Possibly you meant triple equally check? https://github.com/solidjs/solid/blob/main/packages/solid/store/src/mutable.ts#L29