createMutable intensive testing

[titoBouzout]

Describe the bug

I've written a mutable primitive for my lib pota, collected tests for it from solid, oby, vue, + additions/variations I made up. I then ran the tests against the implementations on our libs: solid, oby and pota, to highlight the differences and possible bugs.

There are 240~ tests, I will list below the failing test for solid, around 35~. "by design" implementation details that fail won't count.

1. object returned by function call in mutable is not observed. https://playground.solidjs.com/anonymous/4d40b7dd-48a1-4f73-a6ca-bd98c2b8331f - ref 10 mutation: returned object by function call is mutable

2. writing to an object with setter/getter overwrites getter. https://playground.solidjs.com/anonymous/4c42006f-c124-4054-a74d-70974e0acbb4 - ref 12 getters: returning object

3. In solid frozen objects are made mutable, when a getter returns a frozen object it throws https://playground.solidjs.com/anonymous/4c62129a-5560-475b-b8fa-45e45a96f2b8 - 14 - getters: returning frozen object

4. setting to undefined shouldn't delete the property https://playground.solidjs.com/anonymous/6f1021b5-8e71-49a9-b376-1b89b64b010d - 22 - setting to undefined shouldnt delete the property

5. deleting key that doesn't exists triggers reactivity https://playground.solidjs.com/anonymous/2d07b152-75de-4ea9-a085-c1c5338a2c8f - 24 - delete non existent key doesnt trigger reactivity - object.keys

6. delete key with undefined value doesnt trigger reactivity with check "in" https://playground.solidjs.com/anonymous/5c6627a9-4665-446e-96b4-31b697a77018 delete key with undefined value does trigger reactivity - in

7. attempting to set a value when its only a getter access getter https://playground.solidjs.com/anonymous/44d261b9-70de-452d-8eac-9cc9dd9947a0 - 39 - in: getters to not be called 3

8. access getters more than it should https://playground.solidjs.com/anonymous/fe232aba-ecd7-47ff-83f2-2fbc0633b858 - 40 - in: getters to not be called 3.1

9. access getters more than it should 2 https://playground.solidjs.com/anonymous/cd367536-5769-4083-9cba-90d2ba3c9aa3 - 41 - in: getters to not be called 4

10. problems with getters defined in classes

• https://playground.solidjs.com/anonymous/46e40618-0655-426b-81b0-4d313dad470d - 49 - read and set class
• https://playground.solidjs.com/anonymous/10f2b817-5eff-4ec9-b3c9-e6a5fe15dc26 - 51 - read and set inside class
• https://playground.solidjs.com/anonymous/deb37164-7105-438e-8657-99ab78f11a37 - 52 - read and set inside extended class
• https://playground.solidjs.com/anonymous/488709da-0ea6-4574-aaab-3e1ccf707385 - 53 - read and set inside extended x2 class

11. doesnt react to hasOwnProperty https://playground.solidjs.com/anonymous/da5b1fb6-2914-4fdb-9bb0-ea65f8bdec38 - 55 - reacts to hasOwnProperty

12. throws when redefining hasOwnProperty https://playground.solidjs.com/anonymous/78fb154e-388f-4f45-9335-07e855e9d644 - 79 - returns unproxied properties

13. deleting test

• https://playground.solidjs.com/anonymous/8b1eaf25-9e8c-4d1b-b17b-03e542698d7c - 85 - supports not reacting when deleting a shallow property that was undefined
• https://playground.solidjs.com/anonymous/3505ca38-e13e-466e-8005-612353810737 - 88 - supports not reacting when deleting a deep property that was undefined

14. seems like its over reacting https://playground.solidjs.com/anonymous/ea8df396-5b69-402d-89a1-67a149bc53d0 - 94 - supports reacting to own keys deep
15. deleting

• https://playground.solidjs.com/anonymous/b25dba78-20d8-42f6-a725-37acf763b5de 108 - supports reacting to property checks when value is undefined, deleting
• https://playground.solidjs.com/anonymous/9e72b099-a67d-4a26-9f2f-9b0938228b4f - 109 - supports reacting to property checks when value is undefined, deleting deep

16. reacting to adding properties https://playground.solidjs.com/anonymous/d4304507-190a-4097-846d-a4429108e7d9 - 110 - supports reacting to property checks, adding

17. reacting to adding properties deep https://playground.solidjs.com/anonymous/7c7af674-159c-4707-82be-5a831d30d21d - 111 - supports reacting to property checks, adding deep

18. throws with frozen objects https://playground.solidjs.com/anonymous/3e6b2e6e-7b78-4730-be4b-7e33ca6160fa - 122 - should not observe non-extensible objects

19. reacting to properties from the prototype https://playground.solidjs.com/anonymous/da7072f0-6a88-4a03-bbb8-b5ab9a642562 - 131 - should observe properties on the prototype chain

20. reacts when value changes from NaN to NaN (they probably should be using equals, but well) https://playground.solidjs.com/anonymous/8986df2c-9631-4f49-880c-c2e819c81aa9 - 147 - should not be triggered when the value and the old value both are NaN

21. splice and object identity https://playground.solidjs.com/anonymous/4a0a23b2-2131-47ef-bb0c-d604abeea1f8 - 175 - array: sliced test

22. infinite loop https://playground.solidjs.com/anonymous/f5403e80-7f3f-438f-8b94-fb3b5c29c81a 187 - array: pushing in two separated effects doesnt loop

23. identity test https://playground.solidjs.com/anonymous/cd68b1b1-6a7d-425e-ad11-e3b901fd77c9 205 - array: observed value should proxy mutations to original

24. array methods for searching wont find stuff https://playground.solidjs.com/anonymous/9ded9dba-aa62-43f4-8277-82edd361f523 - 206 - array: identity methods should work

25. more array identity tests https://playground.solidjs.com/anonymous/8f421b42-da3e-4bd1-937a-06d261690a44 - 208 - array: internal array functions should search for the mutable versions of it

26. infinite loop in array https://playground.solidjs.com/anonymous/499412e1-b975-4240-82fc-c440a20325e0 - 224 - array: should avoid infinite recursive loops when use Array.push/unshift/pop/shift

27. array returns past value? something is wrong with batching? https://playground.solidjs.com/anonymous/4e566456-5001-4147-b73c-5e188ac64270 - 226 - array: vue array instrumentation: concat

28. Possible related to batching https://playground.solidjs.com/anonymous/b5d60b40-a558-4828-a586-168b12aed411 - 93 - supports reacting to own keys

Your Example Website or App

https://pota.quack.uy/Reactivity/mutable-tests

Steps to Reproduce the Bug or Issue

Expected behavior

Screenshots or Videos

No response

Platform

• OS: [e.g. macOS, Windows, Linux]
• Browser: [e.g. Chrome, Safari, Firefox]
• Version: [e.g. 91.1]

Additional context

https://github.com/potahtml/pota

A playground, testing all our libs at the same time is in
https://pota.quack.uy/Reactivity/mutable-tests

[ryansolid]

This is a great list that are worth reviewing. Thank you.

[westbrma]

I just tested a Map in a class that is wrapped with createMutable, changes to the map are not reactive, this may also be the same for a Set object. I was doing a direct comparison with Mobx, I am curious why createMutable is not promoted more, I never cared for all the boilerplate around using and setting values, mobx and createMutable abstract all that away from me and generally just works.

[ryansolid]

I just tested a Map in a class that is wrapped with createMutable, changes to the map are not reactive, this may also be the same for a Set object. I was doing a direct comparison with Mobx, I am curious why createMutable is not promoted more, I never cared for all the boilerplate around using and setting values, mobx and createMutable abstract all that away from me and generally just works.

Yeah Map and Set aren't handled. Any special type of object would need pretty special handling because the change mechanisms are internalized. The APIs for change are specific not assignment. We'd need to know how to handle them specifically. Like in MobX they have special classes for those. But if those why not something else. createMutable in general can be dangerous because the ability to mutate is implicit at any depth. Like you can pass parts of it around and implicitly give the ability to mutate it. Big proponent of read/write segregation around here.

[westbrma]

Because of these limitation I have continued to use Mobx with Solid, however I came across a big bug today. You created an example here on using mobx with Solid https://t.co/xu9JrrpBn5
If you reference any reactive property inside the component constructor (the code before the JSX) it will run the entire component again including the constructor code. In the example just add this line console.log(timer.secondsPassed) in the constructor.

Also regarding the danger of mutating deep state anywhere, I'm not sure I would call it implicit, using the = assignment operator is an explicit statement. No one bothers with all the setter boilerplate in backend code, why is it different in frontend code? Maybe because it will re-render UI components?

[fabiospampinato]

@westbrma I can't reproduce the problem in the playground: https://playground.solidjs.com/anonymous/fff085cd-0234-4958-bf82-7cad295847dc

While I can reproduce it in the codesandbox with the same code, so I guess Babel is configured improperly in that codesandbox or something.

[westbrma]

@westbrma I can't reproduce the problem in the playground: https://playground.solidjs.com/anonymous/fff085cd-0234-4958-bf82-7cad295847dc

While I can reproduce it in the codesandbox with the same code, so I guess Babel is configured improperly in that codesandbox or something.

@fabiospampinato
strange, however you can reproduce if you click export to zip in your playground, then do
npm install
npm run start

[trusktr]

@westbrma to make a reactive Map you'd have to make a subclass of Map, and extend all the methods to read and write to/from Solid signals (or a store). Here's an example:

https://github.com/lume/element-behaviors/blob/main/src/BehaviorMap.ts