search

solidjs / solid

A declarative, efficient, and flexible JavaScript library for building user interfaces.
https://solidjs.com
MIT License
32.17k stars 919 forks source link

`createMutable` intensive testing #2112

Open titoBouzout opened 6 months ago

titoBouzout commented 6 months ago

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

  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

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

  5. deleting

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

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

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

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

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

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

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

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

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

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

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

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

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

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 commented 5 months ago

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

westbrma commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

@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 commented 2 weeks ago

@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