solid icon indicating copy to clipboard operation
solid copied to clipboard
verified publisher icon solidjs

`createMutable` intensive testing

Open titoBouzout opened this issue 1 year ago • 12 comments

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

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

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

  3. 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
  1. seems like its over reacting https://playground.solidjs.com/anonymous/ea8df396-5b69-402d-89a1-67a149bc53d0 - 94 - supports reacting to own keys deep
  2. 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

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

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

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

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

  5. 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

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

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

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

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

  10. 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

  11. 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

  12. 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

  13. 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

titoBouzout avatar Mar 28 '24 21:03 titoBouzout

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

ryansolid avatar Apr 08 '24 17:04 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.

westbrma avatar Jun 21 '24 11:06 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.

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.

ryansolid avatar Jun 21 '24 15:06 ryansolid

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?

westbrma avatar Jul 03 '24 11:07 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 avatar Jul 03 '24 11:07 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.

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

westbrma avatar Jul 03 '24 12:07 westbrma

@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

trusktr avatar Sep 17 '24 11:09 trusktr